Pytest discovery using new Python test discovery adapter. #4695
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
DonJayamanne
requested changes
Mar 9, 2019
There was a problem hiding this comment.
I think the python API needa to be re-visited.
We are still parsing strings and building a tree, in the TS layer to build and identify parent child relationships.
This needs to be done in the Python layer where the entire structure of known and prescribed, i.e. it should simply return a strongly typed json structure = a tree, making it totally unnecessary to build a tree/parse again in TS.
Codecov Report
@@ Coverage Diff @@
## master #4695 +/- ##
========================================
- Coverage 77% 62% -15%
========================================
Files 447 372 -75
Lines 21472 14476 -6996
Branches 3527 1145 -2382
========================================
- Hits 16444 8860 -7584
- Misses 5024 5413 +389
- Partials 4 203 +199
|
- first pass of the new test discovery API exec - trying to not change anything outside of unittest - particularly outside of pytest
- Make the run_adapter placeholder return an array - use JSON.Parse on the returned result in parser
- Add a script that just spits out test data
- Use python.pythonPath for the exe path - Put the run_adapter.py script as the first arg to the process - Add the rest of the necessary args to the script.
- Add `discover` method alongside `run` method - Focusing on running discovery via the new Python adapter - This will be the call for each test framework going forward
- New data type expected from Python `DiscoveredTestData` - New class to transform from DiscoveredTestData to Tests - Updated `pytest/parserService.ts` to use this new method of discovery
- A bit of a logic re-jig
- start testing to ensure the new discovery works the same as the old
- Fix up test data for path case variance.
- Fix bug with parser throwing on empty input - Tests for the discoveredTests - Tests fixed for the discoveryService - simplify tests for the parserService - remove a non-unit test from running during unit test runs
- Use `exec` over `execObservable` in running Python for test discovery. - Create a new interface for test discovery runner.
- Still needs some fixing.
d3r3kk
force-pushed
the
4035_pytest_discovery
branch
from
7f7cf5f
to
8ad18c1
Compare
This reverts commit 8ad18c1.
- needs to be refactored for the new parser model - tracked by GH microsoft#4735
DonJayamanne
approved these changes
Mar 13, 2019
- Many tests will need refactoring with the new discovery API
DonJayamanne
requested changes
Mar 13, 2019
There was a problem hiding this comment.
Please do not merge:
- File names, folder names, suite names and function names are not returned by python code (new PR Pytest discovery using new Python test discovery adapter. #4695 will solve this)
- Extracting names from IDs is not the right approach (that's business logic that needs to sit in python code, as its the python code that knows how ids are formed) (new PR Pytest discovery using new Python test discovery adapter. #4695 will solve this)
- Id is unique identifier, and we shouldn't assume it contains names of files, suites and functions
As a result of this:
- Code lenses will not work (line numbers returned by python code is not used, as user may have modified the code with auto test discover disabled)
- Selection of tests for running will not display user friendly names
Summary:
- We need to get python code to return the names
- Else we leave code in broken state, that should not be done in PRs
- We need to wait for PR Pytest discovery using new Python test discovery adapter. #4695
DonJayamanne
requested changes
Mar 13, 2019
| if parts.pop(0) != filename: | ||
| # TODO: What to do? | ||
| if os.path.normcase(parts.pop(0)) != filename: | ||
| # The filename of this node doesn't match the filename within the node id? |
There was a problem hiding this comment.
We need to file an issue about this and fix this accordingly.
/cc @ericsnowcurrently
| pytestargs.insert(0, '-pno:terminal') | ||
| pytestargs.insert(0, '--collect-only') | ||
| pytestargs.insert(0, '--cache-clear') |
| @@ -0,0 +1,94 @@ | |||
| import json | |||
|
@d3r3kk |
8 tasks
|
@d3r3kk |
This was referenced Mar 18, 2019
DonJayamanne
added a commit
that referenced
this pull request
Apr 11, 2019
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
For #4035
Transform results from our new Python test adapter for discovery into the
Testsstruct we use throughout the extension.Test plan is updated as appropriatepackage-lock.jsonhas been regenerated by runningnpm install(if dependencies have changed)