-
Notifications
You must be signed in to change notification settings - Fork 18
Integration tests for livelog and fix for s3 upload #373
Conversation
Now this also fixes empty artifact upload, issue #375 |
// If we haven't made a superseded task yet we do that now | ||
if supersededTaskID == "" { | ||
supersededTaskID = taskID | ||
// Make first other taskId be superseding this taskId |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't understand this sentence
{ | ||
"name": "public/result.txt", | ||
"type": "file", | ||
"path": "no-such-file-` + filename + `.txt" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is the variable filename defined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably higher up...this is just a mechanically refactored test case.. see the commit
test/scripttest/logging_test.go
Outdated
if goruntime.GOOS == "windows" { | ||
// On windows this fails with: | ||
// dial tcp 127.0.0.1:49523: socket: The requested service provider could not be loaded or initialized. | ||
// Calling the URL from this process works, but the subprocess fails... | ||
t.Skip("TODO: Fix the GetUrl on windows, probably firewall interference") | ||
} | ||
|
||
var u string | ||
// Generate some messages that can be written log, so we can read them from livelog |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: "... can be written log ..."
test/scripttest/logging_test.go
Outdated
"very-long": base64.StdEncoding.EncodeToString(randMessage), | ||
} | ||
|
||
// Generate some replies that can be written log, so we know that test-server was called |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: "... can be written log ..."
test/scripttest/logging_test.go
Outdated
workertest.Case{ | ||
Engine: "script", | ||
Concurrency: 1, | ||
Concurrency: 5, // this test runs a lot of task, we better run them in parallel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: "a lot of tasks"
taskID := r.URL.Query().Get("taskId") | ||
msg := messages[r.URL.Query().Get("msgSize")] | ||
behavior := r.URL.Query().Get("behavior") | ||
reply := replies[r.URL.Query().Get("replySize")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are msg
and reply
buffers or buffer sizes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are strings... messages
is a mapping from string representing size to message as string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is somewhat confusing IMO, but we can talk and address this in another PR
test/scripttest/logging_test.go
Outdated
} | ||
} | ||
|
||
// Signal that eveyrthing went well, and return reply. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: "everything"
worker/workertest/workertest.go
Outdated
@@ -33,7 +33,7 @@ type Case struct { | |||
Concurrency int // Worker concurrency, if zero defaulted to 1 and tasks will sequantially dependent | |||
StoppedGracefully bool // True, if worker is expected to stop gracefully | |||
StoppedNow bool // True, if worker is expected to stop now | |||
Timeout time.Duration // Test timeout, defaults to 10 Minute | |||
Timeout time.Duration // Test timeout, defaults to 8 Minute |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: "Minutes"
worker/workertest/workertest.go
Outdated
} | ||
|
||
// Create task asynchronously with a limit of 10 concurrent calls to make | ||
// that it goes fairly faster, when there is a lot tasks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: "a lot of"
worker/workertest/workertest.go
Outdated
tdefs[i].Metadata.Owner = "[email protected]" | ||
} | ||
|
||
// Create task asynchronously with a limit of 10 concurrent calls to make |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: "tasks"
@@ -529,6 +529,16 @@ func (q *FakeQueue) internalPutArtifact(taskID string, runID int, name string, p | |||
// Real S3/azure is slow, let's add a bit of jitter to get some intermittent bugs | |||
time.Sleep(time.Duration(rand.Intn(15)) * time.Millisecond) // sleep 0 - 15ms | |||
|
|||
// S3 always requires a content-length, if it's not present we have bug |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: "have a bug"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The commits then should be squashed
8b8a29a
to
0256c2c
Compare
0256c2c
to
57ccd6f
Compare
@walac, I added a fix for the broken tests in master... due to missing test fixes from the I also rebased and merged a few of the commits... So each commit does one thing. |
No description provided.