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 discovery of PyTests with packages inbetween folders #3937

Closed
wants to merge 1 commit into from

Conversation

Flamefire
Copy link

For #3936

  • Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR)
  • Title summarizes what is changing
  • Has a news entry file (remember to thank yourself!)
  • Unit tests & system/integration tests are added/updated
  • Test plan is updated as appropriate
  • [x]~ package-lock.json has been regenerated by running npm install (if dependencies have changed)~

@msftclas
Copy link

msftclas commented Jan 9, 2019

CLA assistant check
All CLA requirements met.

@codecov
Copy link

codecov bot commented Jan 9, 2019

Codecov Report

Merging #3937 into master will increase coverage by 1%.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #3937   +/-   ##
======================================
+ Coverage      78%     78%   +1%     
======================================
  Files         402     402           
  Lines       18568   18574    +6     
  Branches     2983    2985    +2     
======================================
+ Hits        14350   14356    +6     
  Misses       4216    4216           
  Partials        2       2
Flag Coverage Δ
#Linux 68% <59%> (ø) ⬇️
#Windows 68% <59%> (ø) ⬇️
#macOS 68% <59%> (ø) ⬇️
Impacted Files Coverage Δ
.../client/unittests/pytest/services/parserService.ts 81% <100%> (+1%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4c2655f...ee596a6. Read the comment docs.

@Flamefire
Copy link
Author

@DonJayamanne It passes all but 1 test where the remaining one is a download failure on travis. Could you have a look at the code?

@d3r3kk d3r3kk added this to the 2019, week 3 - Jan Sprint 2 milestone Jan 17, 2019
@DonJayamanne
Copy link

@Flamefire
Thanks for the PR, unfortunately the latest version of PyTest breaks all of the test discovery and we have plans to change this code. I.e. even if we merged your changes, it will become redundant this sprint.
The issue used to re-write the discovery in python is being tracked in #4033

@Flamefire
Copy link
Author

@DonJayamanne any update on this? I've seen #4098 was merged containing a fix for pytest 4.1 based on the current discovery and #3911 is no longer a P1 issue. Seems like the refactoring won't happen soon and discovery stays broken for e.g. my use case making this unusable.

@DonJayamanne
Copy link

Seems like the refactoring won't happen soon and discovery stays broken for e.g. my use case making this unusable.

Thank continue in the next Sprint (next week). Work has started on it, but as of on hold for this Sprint.

@Flamefire
Copy link
Author

Ah ok. Then maybe I misunderstood the significance of removing it from P1 as "P1" was described as "mandatory for next release" and no P1 or P2 as "may or may not come".

So will the fix be included in the next release? If not, do you consider taking my PR instead? It allows (at least) me to use the extension again for tests, has tests and is updated to work with the new pytest version (supported after #4098). It does not create conflicts with the upcoming fix as the whole code will be removed/replaced by the new version using a "native parser" (if I may call it like that)

@lock lock bot locked as resolved and limited conversation to collaborators Jul 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants