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

fix(job_attachments): improvements to nonvalid error messages #200

Merged
merged 1 commit into from
Mar 7, 2024

Conversation

gahyusuh
Copy link
Contributor

@gahyusuh gahyusuh commented Mar 7, 2024

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

We noticed a flaky unit test in the job attachments part of the package: https://github.com/casillas2/deadline-cloud/actions/runs/8165059251/job/22321551354
The reason it was flakey is because (I think) it sometimes throw this AssetSyncError, and sometimes it throws this AssetSyncError from different place, and the former includes the phrase “Invalid value for configuration setting: ” in the error message, but the latter does not. So it can fail in the assert that checks if the phrase is included in the error message.

What was the solution? (How)

  1. Added the phrase "Nonvalid value for configuration setting: ” in the AssetSyncError thrown by the get_s3_max_pool_connections().
  2. Added a couple more test cases.
  3. Fixed the term "invalid" to "nonvalid" to avoid using non-Inclusive language.

What is the impact of this change?

The tests should be less flaky.

How was this change tested?

Ran hatch run lint && hatch run test and made sure all passed.

Was this change documented?

No.

Is this a breaking change?

No.

@gahyusuh gahyusuh marked this pull request as ready for review March 7, 2024 20:19
@gahyusuh gahyusuh requested a review from a team as a code owner March 7, 2024 20:19
@jusiskin
Copy link
Contributor

jusiskin commented Mar 7, 2024

@gahyusuh: the commit/PR title is used to generate the changelog. Can we adjust the message to reflect the functional changes?

@gahyusuh gahyusuh changed the title fix: flakey unit tests for handling nonvalid config settings fix: add phrase "Nonvalid value for configuration setting" to error message Mar 7, 2024
@gahyusuh gahyusuh changed the title fix: add phrase "Nonvalid value for configuration setting" to error message fix: improvements to nonvalid error messages Mar 7, 2024
Fixed a flakey unit test for handling nonvalid config settings.

Signed-off-by: Gahyun Suh <132245153+gahyusuh@users.noreply.github.com>
@gahyusuh gahyusuh changed the title fix: improvements to nonvalid error messages fix(job_attachments): improvements to nonvalid error messages Mar 7, 2024
@jusiskin jusiskin merged commit 148587a into mainline Mar 7, 2024
18 checks passed
@jusiskin jusiskin deleted the gahyusuh/fix_flakey_tests branch March 7, 2024 21:04
@lucaseck lucaseck mentioned this pull request Mar 11, 2024
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