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: adaptor wheel error dialog and unit test coverage #18

Merged
merged 2 commits into from
Sep 19, 2023

Conversation

tincheng
Copy link
Contributor

@tincheng tincheng commented Sep 11, 2023

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

  • Unit test coverage needed improvement
  • There was a small bug where if we tried to submit without setting up wheel directory but had "developer options: include adaptor wheels" selected, then it gave a generic FileNotFoundError instead of the intended The Developer Option 'Include Adaptor Wheels' is enabled, but the wheels directory does not exist message

What was the solution? (How)

  • increased unit test coverage to ~77%
  • changed the check for directory to rely on just is_dir() instead of exists()

What is the impact of this change?

Better test coverage and small fix for user experience

How was this change tested?

  • coverage updated from hatch run test
  • locally verified that intended Deadline error message was shown in UI instead of FileNotFound error

Did you run the "Job Bundle Output Tests"? If not, why not? If so, paste the test results here.

No - I was not able to run the Job Bundle Output Tests on Mac. Got Failed 2 tests, succeeded 0. See the file /Users/tincheng/workplace/deadline-cloud-for-nuke/job_bundle_output_tests/test-job-bundle-results.txt for a full report. and the output file was blank. Probably need more time to get the bundle tests working on Mac, but I think this PR should have low impact without this as changes are either just unit tests or had been verified within DCC.

Required: paste the contents of job_bundle_output_tests/test-job-bundle-results.txt here

Was this change documented?

No

Is this a breaking change?

No

@tincheng tincheng requested a review from a team as a code owner September 11, 2023 22:56
@tincheng tincheng changed the title Tincheng/unit test coverage add unit test coverage Sep 11, 2023
@tincheng tincheng force-pushed the tincheng/unit_test_coverage branch 4 times, most recently from 3a67d4d to 286dc15 Compare September 12, 2023 00:32
@tincheng tincheng changed the title add unit test coverage fix: adaptor wheel error dialog and unit test coverage Sep 12, 2023
Copy link
Contributor

@rmv rmv left a comment

Choose a reason for hiding this comment

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

LGTM (though I'm new to this testing setup)

@@ -673,3 +730,34 @@ def test_does_nothing_if_nuke_not_running(
# THEN
assert "CANCEL REQUESTED" in caplog.text
assert "Nothing to cancel because Nuke is not running" in caplog.text


class TestNukeAdaptor_get_major_minor_version:
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing type hints in these methods

@tincheng tincheng force-pushed the tincheng/unit_test_coverage branch 2 times, most recently from 9c754ca to 36af21e Compare September 18, 2023 22:46
tincheng and others added 2 commits September 18, 2023 15:56
Signed-off-by: Ting Cheng <36546476+tincheng@users.noreply.github.com>
Signed-off-by: Ting Cheng <36546476+tincheng@users.noreply.github.com>
@mwiebe mwiebe merged commit 1fcdec4 into mainline Sep 19, 2023
5 checks passed
@mwiebe mwiebe deleted the tincheng/unit_test_coverage branch September 19, 2023 01:04
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.

6 participants