Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Makes several tests more reliable #1321

Merged
merged 2 commits into from Nov 27, 2019

Conversation

@dposada
Copy link
Member

dposada commented Nov 27, 2019

Changes proposed in this PR

See comments inline with changes.

Why are we making these changes?

See comments inline with changes.

@dposada dposada self-assigned this Nov 27, 2019
@dposada dposada changed the title Makes test_basic_docker_job more reliable Makes several tests more reliable Nov 27, 2019
@@ -675,7 +675,7 @@ def memory_limit_exceeded_helper(self, command, executor_type, mem=128):
job_details = f"Job details: {json.dumps(job, sort_keys=True)}"
self.assertEqual('failed', job['state'], job_details)
self.assertLessEqual(1, len(job['instances']), job_details)
instance = job['instances'][-1]
instance = next(i for i in job['instances'] if i['reason_code'] in [2002, 99003])

This comment has been minimized.

Copy link
@dposada

dposada Nov 27, 2019

Author Member

Instead of grabbing the "last" instance, grab the instance with the expected instance code.

We should never assume that a particular instance (by index) of a job is the one we're interested in.

# complete; otherwise we could get a false-negative on wait_for_job 'completed'
# because of a scheduling delay. Having two separate waits also disambiguates
# the root cause of a wait-timeout failure.
util.wait_for_job_in_statuses(self.cook_url, job_uuid, ['completed', 'running'])

This comment has been minimized.

Copy link
@dposada

dposada Nov 27, 2019

Author Member

Wait for the job to start running, and only then start waiting for it to complete; otherwise we could get a false-negative on wait_for_job 'completed' because of a scheduling delay.

@@ -1690,7 +1695,8 @@ def test_basic_docker_job(self):
self.cook_url,
command='cat /.dockerenv',
container={'type': 'DOCKER',
'docker': {'image': image}})
'docker': {'image': image}},
max_retries=5)

This comment has been minimized.

Copy link
@dposada

dposada Nov 27, 2019

Author Member

Let the job retry, so that a single failed instance does not cause the test to fail.

@dposada dposada marked this pull request as ready for review Nov 27, 2019
@dposada dposada requested a review from shamsimam Nov 27, 2019
@shamsimam shamsimam merged commit 94a26b8 into twosigma:master Nov 27, 2019
2 checks passed
2 checks passed
Mergeable Mergeable Run has been Completed!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@shamsimam shamsimam deleted the dposada:test-basic-docker-job-retries branch Nov 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.