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

Reveal and fix a masked test #1671

Merged
merged 3 commits into from
Jul 25, 2022
Merged

Reveal and fix a masked test #1671

merged 3 commits into from
Jul 25, 2022

Conversation

rahulporuri
Copy link
Contributor

@rahulporuri rahulporuri commented Jul 17, 2022

The test name started with an underscore which is why unittest wasnt discovering it.

Once the leading underscore is removed, we noticed that the test assertion fails - because the app_name that the code infers by default is different.

We update the assertion to pass - with the strong assumption that the tests are run using unittest - which traits uses for test discovery and to run the tests.

Checklist

  • Tests
  • Update API reference (docs/source/traits_api_reference)
  • Update User manual (docs/source/traits_user_manual)
  • Update type annotation hints in traits-stubs

the test method started with an underscore which is why unittest wasnt
discovering it

	modified:   traits/etsconfig/tests/test_etsconfig.py
@rahulporuri
Copy link
Contributor Author

The test failed as expected -

======================================================================
FAIL: test_default_application_home (traits.etsconfig.tests.test_etsconfig.ETSConfigTestCase)
application home
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/opt/hostedtoolcache/Python/3.7.13/x64/lib/python3.7/site-packages/traits/etsconfig/tests/test_etsconfig.py", line 218, in test_default_application_home
    self.assertEqual(app_name, "tests")
AssertionError: 'unittest' != 'tests'
- unittest
+ tests
----------------------------------------------------------------------

Fixing it now.

Poruri Sai Rahul added 2 commits July 17, 2022 12:46
we are making a strong assumption here that the tests are being run
using unittest

	modified:   traits/etsconfig/tests/test_etsconfig.py
	modified:   traits/etsconfig/tests/test_etsconfig.py
@rahulporuri rahulporuri changed the title [WIP] Reveal and fix a masked test method Reveal and fix a masked test method Jul 17, 2022
@rahulporuri rahulporuri changed the title Reveal and fix a masked test method Reveal and fix a masked test Jul 17, 2022
@mdickinson mdickinson self-requested a review July 19, 2022 09:10
Copy link
Member

@mdickinson mdickinson left a comment

Choose a reason for hiding this comment

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

LGTM as a quick fix, though having the validity of the test tied to the choice of test runner isn't ideal.

Probably we should rethink the whole way that application_data gets its default.

@corranwebster
Copy link
Contributor

Probably we should rethink the whole way that application_data gets its default.

I have opened #1672 for this.

@mdickinson mdickinson merged commit f379908 into main Jul 25, 2022
@mdickinson mdickinson deleted the tst/reveal-masked-test branch July 25, 2022 07:50
mdickinson pushed a commit that referenced this pull request Aug 9, 2022
* DEV: Reveal a masked test method

the test method started with an underscore which is why unittest wasnt
discovering it

	modified:   traits/etsconfig/tests/test_etsconfig.py

* FIX: Update the app_name and get the test passing

we are making a strong assumption here that the tests are being run
using unittest

	modified:   traits/etsconfig/tests/test_etsconfig.py

* CLN: Remove the now unnecessary dundername/dundermain clause

	modified:   traits/etsconfig/tests/test_etsconfig.py

(cherry picked from commit f379908)
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.

None yet

3 participants