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

test: Add test to verify WA job attachment settings #373

Merged
merged 2 commits into from
Aug 7, 2024

Conversation

YutongLi291
Copy link
Contributor

What was the problem/requirement? (What/Why)

We want to verify that worker agent is able to follow queues job attachment settings and correctly identify input/output directories to take actions on files.

We should add testing regarding this functionality.

What was the solution? (How)

Add a test that verifies that WA is able to execute a job that respects the job attachment settings, and verifies that the job output is as we expected.

What is the impact of this change?

Better verification on the functionality to use job attachments with jobs, as well as with Worker Agent.

How was this change tested?

hatch run linux-e2e-tests

Was this change documented?

no

Is this a breaking change?

no

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@YutongLi291 YutongLi291 requested a review from a team as a code owner August 7, 2024 03:14
test/e2e/linux/test_job_submissions.py Show resolved Hide resolved
Comment on lines 131 to 135
job_bundle_path: str = os.path.join(
os.path.dirname(__file__),
"..",
"..",
"..",
"scripts",
"submit_jobs",
"job_attachment_job",
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest putting the test job beside this file, rather than back up in the scripts directory. Reason being that the scripts directory is intended to be a fairly random collection of useful scripts and files for manual testing. Putting a part of the e2e tests in there risks someone changing the bundle in some way that's unexpected by the test. Yes, the test should catch that, but it's a better idea to have the testing files all within the testing directories, imo.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, I've put the job bundle under the tests path


assert output_file_content == (input_file_content + test_run_uuid)

if os.path.exists(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than this, I'd suggest downloading the output files into a temporary directory. Reason being that a failed test will leave files behind right now -- e.g. if the output_file.txt doesn't get generated, or downloaded, for some reason then the open call above will raise an exception and abort the test.

If you use a tempfile.TemporaryDirectory as a context manager, instead, then the tempdir will get deleted regardless of whether the test exists successfully or not.

Alternatively to that, there's also the tmp_path fixture built-in to pytest for getting a temporary directory for the duration of a test: https://docs.pytest.org/en/stable/how-to/tmp_path.html#the-tmp-path-fixture

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed it to use a temp directory in the revision. Thanks for pointing this out

Signed-off-by: Yutong Li <52769999+YutongLi291@users.noreply.github.com>
Copy link
Contributor

@ddneilson ddneilson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding this test!

@YutongLi291 YutongLi291 enabled auto-merge (squash) August 7, 2024 19:59
Copy link

sonarcloud bot commented Aug 7, 2024

@YutongLi291 YutongLi291 merged commit 2c45a33 into aws-deadline:mainline Aug 7, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants