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

Initial unit testing framework and tests #7026

Merged
merged 6 commits into from Apr 12, 2017

Conversation

Projects
None yet
3 participants
@jcsteh
Contributor

jcsteh commented Mar 29, 2017

This adds an initial unit testing framework, including necessary changes for SCons and AppVeyor. It also adds some tests for the cursorManager module, which serve both as useful tests and as an example of how this can be used.

This is far from perfect. In particular, there is some pretty ugly bootstrapping code, and because NVDA wasn't originally designed with unit testing in mind, tests aren't isolated from side effects. However, we need to start somewhere and I think we can improve these things over time. Most of the ugliness is in one file and we have a base TestCase so we can provide base setUp and tearDown methods in order to better isolate tests.

jcsteh added some commits Mar 28, 2017

Add initial unit testing framework.
Unit tests and infrastructure are located in the tests/unit directory. See the docstring in the tests/unit/__init__.py file for details.
You can run tests using "scons tests". See the "Running Tests" section of readme.md for details.
Fix "'NoneType' object is not callable" exceptions in WavePlayer.__de…
…l__ when unit tests finish (and when NVDA exits).

These have existed for a long time, but they aren't normally noticed when exiting NVDA, since we normally don't see stderr when exiting.
To fix this, nvwave and tones now register an atexit callback to clean up their WavePlayer instances before modules get cleaned up.
Fix some obscure build system issues which were causing some idl and …
…comInterfaces files to be rebuilt unnecessarily.

This is not a new problem, but it's particularly annoying for automated testing because running the tests often causes files to be rebuilt unnecessarily.
1. The ISimpleDOM and MathPlayer interfaces have multiple idl files. The targets for the idl file have an implicit dependency on midl.exe, but its position in the dependencies is different in the run before the idl files are copied versus subsequent runs. I think this is an SCons bug/quirk, probably because before the copy, the idl file must be scanned after the midl tool is considered. Explicitly specifying these extra idls as dependencies doesn't help. The only workaround I've found is to ignore midl.exe as a dependency when checking whether the targets should be rebuilt.
2. comInterfaces generates .pyc as well as .py files. We now tell SCons about the .pyc files as well so they get considered appropriately. Previously, the .py file could be removed, so SCons would rebuild the .py file. However, the .pyc still existed, so it was just loaded without generating a .py file. Subsequently, SCons would try to rebuild the .py file on every build.

@jcsteh jcsteh requested a review from feerrenrut Mar 29, 2017

@@ -39,3 +39,4 @@ uninstaller/UAC.nsh
*.pyc
*.pyo
*.dmp
tests/unit/nvda.ini

This comment has been minimized.

@feerrenrut

feerrenrut Mar 31, 2017

Contributor

This is probably fine for now. But in the future this might need to pulled into the working directory for the test (even better if it was in memory rather than moving actual files around) during the setup for the test. If all tests use the same config file, there is a chance that some test will modify the settings and we could get some pretty nasty test interactions.

@feerrenrut

feerrenrut Mar 31, 2017

Contributor

This is probably fine for now. But in the future this might need to pulled into the working directory for the test (even better if it was in memory rather than moving actual files around) during the setup for the test. If all tests use the same config file, there is a chance that some test will modify the settings and we could get some pretty nasty test interactions.

This comment has been minimized.

@jcsteh

jcsteh Mar 31, 2017

Contributor

Totally agree that ideally, we want to keep this in memory and isolate it per test. If it eases your concern any in the short-term, it's worth noting that nothing in NVDA ever really saves the configuration except the save configuration command or auto save on NVDA exit, neither of which should ever be reasonably called from a unit test.

@jcsteh

jcsteh Mar 31, 2017

Contributor

Totally agree that ideally, we want to keep this in memory and isolate it per test. If it eases your concern any in the short-term, it's worth noting that nothing in NVDA ever really saves the configuration except the save configuration command or auto save on NVDA exit, neither of which should ever be reasonably called from a unit test.

# We don't need launcher to run tests, so run the tests before launcher.
- py scons.py tests %sconsArgs%
- 'echo scons output targets: %sconsOutTargets%'
- py scons.py %sconsOutTargets% %sconsArgs%

This comment has been minimized.

@feerrenrut

feerrenrut Mar 31, 2017

Contributor

Each of these lines ending with %sconsArgs% (lines: 76, 78, 80) has different line ending ^M

@feerrenrut

feerrenrut Mar 31, 2017

Contributor

Each of these lines ending with %sconsArgs% (lines: 76, 78, 80) has different line ending ^M

This comment has been minimized.

@jcsteh

jcsteh Mar 31, 2017

Contributor

I don't know why GitHub is flagging this. All of these files have Windows line endings all the way through (as is generally our convention)... and looking at the raw content (outside of an editor), there's nothing special about these lines.

@jcsteh

jcsteh Mar 31, 2017

Contributor

I don't know why GitHub is flagging this. All of these files have Windows line endings all the way through (as is generally our convention)... and looking at the raw content (outside of an editor), there's nothing special about these lines.

Show outdated Hide outdated nvdaHelper/ISimpleDOM_sconscript Outdated
# When exiting, ensure fileWavePlayer is deleted before modules get cleaned up.
# Otherwise, WavePlayer.__del__ will fail with an exception.
@atexit.register

This comment has been minimized.

@feerrenrut

feerrenrut Mar 31, 2017

Contributor

There is a windows line ending here too.

@feerrenrut

feerrenrut Mar 31, 2017

Contributor

There is a windows line ending here too.

@@ -20,6 +21,13 @@
log.warning("Failed to initialize audio for tones")
player = None
# When exiting, ensure player is deleted before modules get cleaned up.
# Otherwise, WavePlayer.__del__ will fail with an exception.
@atexit.register

This comment has been minimized.

@feerrenrut

feerrenrut Mar 31, 2017

Contributor

There is also a windows line ending here.

@feerrenrut

feerrenrut Mar 31, 2017

Contributor

There is also a windows line ending here.

Show outdated Hide outdated tests/unit/__init__.py Outdated
Show outdated Hide outdated tests/unit/__init__.py Outdated
self.assertEqual(cm.selectionOffsets, (1, 1)) # Caret at "b"
def test_prevChar(self):
cm = CursorManager(text="abc", selection=(1, 1)) # Caret at "b"

This comment has been minimized.

@feerrenrut

feerrenrut Mar 31, 2017

Contributor

Rather than the comment, the (1,1) could be held in a variable named caretAtB

@feerrenrut

feerrenrut Mar 31, 2017

Contributor

Rather than the comment, the (1,1) could be held in a variable named caretAtB

self.assertEqual(cm.selectionOffsets, (0, 0)) # Caret at "a", no selection
def test_selForwardThenUnselThenSelBackward(self):
"""Test selecting forward, then unselecting and selecting backward.

This comment has been minimized.

@feerrenrut

feerrenrut Mar 31, 2017

Contributor

The docstring could be more explicit about the dependencies between tests / behaviour.

@feerrenrut

feerrenrut Mar 31, 2017

Contributor

The docstring could be more explicit about the dependencies between tests / behaviour.

@jcsteh

This comment has been minimized.

Show comment
Hide comment
@jcsteh

jcsteh Apr 3, 2017

Contributor

As discussed with @michaelDCurran and @feerrenrut, we'll incubate this one for a day or so just to make sure there's no serious breakage in the build or the like. However, we want to get it into master sooner rather than later so we can actually start basing branches off it and getting unit tests. Also, there's no notable code change here for NVDA itself, so this should have no effect for users.

Contributor

jcsteh commented Apr 3, 2017

As discussed with @michaelDCurran and @feerrenrut, we'll incubate this one for a day or so just to make sure there's no serious breakage in the build or the like. However, we want to get it into master sooner rather than later so we can actually start basing branches off it and getting unit tests. Also, there's no notable code change here for NVDA itself, so this should have no effect for users.

@jcsteh jcsteh merged commit 423463e into master Apr 12, 2017

@nvaccessAuto nvaccessAuto added this to the 2017.2 milestone Apr 12, 2017

@jcsteh jcsteh deleted the unitTests branch Apr 12, 2017

jcsteh added a commit that referenced this pull request Apr 12, 2017

jcsteh added a commit that referenced this pull request Apr 12, 2017

Initial unit testing framework and tests. (#7026)
This adds an initial unit testing framework, including necessary changes for SCons and AppVeyor. It also adds some tests for the cursorManager module, which serve both as useful tests and as an example of how this can be used.
Unit tests and infrastructure are located in the tests/unit directory. See the docstring in the tests/unit/__init__.py file for details.
You can run tests using "scons tests". See the "Running Tests" section of readme.md for details.

jcsteh added a commit that referenced this pull request Apr 12, 2017

@jcsteh

This comment has been minimized.

Show comment
Hide comment
@jcsteh

jcsteh Apr 12, 2017

Contributor

Oops. I accidentally squash merged this when I meant to do a normal merge. While we normally squash merge, there is context in these commits which is quite important to figure out why I made some seemingly unrelated changes.

I force pushed to fix this. Just so we have this documented:

  • The old merge was 423463e. The original What's New commit was 8d0fe3d.

  • I undid both of these commits, merged manually, cherry-picked the What's New entry and force pushed.

  • The new merge commit is 948abb1. The new What's New entry commit is 8120e75.

  • I double checked there were no commits between my original What's New commit and my force push.

  • I confirmed that the tree between the old head and the new head is the same:

    $ git diff 8d0fe3df..8120e75c
    $
    
Contributor

jcsteh commented Apr 12, 2017

Oops. I accidentally squash merged this when I meant to do a normal merge. While we normally squash merge, there is context in these commits which is quite important to figure out why I made some seemingly unrelated changes.

I force pushed to fix this. Just so we have this documented:

  • The old merge was 423463e. The original What's New commit was 8d0fe3d.

  • I undid both of these commits, merged manually, cherry-picked the What's New entry and force pushed.

  • The new merge commit is 948abb1. The new What's New entry commit is 8120e75.

  • I double checked there were no commits between my original What's New commit and my force push.

  • I confirmed that the tree between the old head and the new head is the same:

    $ git diff 8d0fe3df..8120e75c
    $
    
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment