Skip to content

Explicitly clarify the difference between unit and system test files#9419

Merged
karrtikr merged 3 commits into
microsoft:masterfrom
PeterJCLaw:clarify-unit-tests-docs
Jan 16, 2020
Merged

Explicitly clarify the difference between unit and system test files#9419
karrtikr merged 3 commits into
microsoft:masterfrom
PeterJCLaw:clarify-unit-tests-docs

Conversation

@PeterJCLaw
Copy link
Copy Markdown

This aims to clarify for new contributors how to run the tests they've may have edited.

  • 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!)
    Not sure this tiny change is worth a news entry

(other bullets don't apply to docs changes)

This aims to clarify for new contributors how to run the tests they've may have edited.
@codecov-io
Copy link
Copy Markdown

codecov-io commented Jan 7, 2020

Codecov Report

Merging #9419 into master will decrease coverage by 0.45%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9419      +/-   ##
==========================================
- Coverage   60.93%   60.47%   -0.46%     
==========================================
  Files         529      544      +15     
  Lines       28578    29313     +735     
  Branches     4338     4439     +101     
==========================================
+ Hits        17413    17727     +314     
- Misses      10214    10595     +381     
- Partials      951      991      +40
Impacted Files Coverage Δ
src/client/linters/mypy.ts 38.46% <0%> (-49.04%) ⬇️
src/client/common/process/condaExecutionService.ts 70% <0%> (-30%) ⬇️
src/client/datascience/jupyter/jupyterVariables.ts 50.59% <0%> (-11.38%) ⬇️
...lient/datascience/jupyter/kernels/kernelService.ts 50.58% <0%> (-10.14%) ⬇️
src/client/common/utils/platform.ts 70.58% <0%> (-5.89%) ⬇️
src/client/common/process/pythonProcess.ts 92.3% <0%> (-4.19%) ⬇️
src/client/interpreter/activation/service.ts 84.93% <0%> (-2.77%) ⬇️
src/client/datascience/jupyter/notebookStarter.ts 69% <0%> (-2.55%) ⬇️
src/client/activation/activationService.ts 88.16% <0%> (-1.6%) ⬇️
...ce/interactive-window/interactiveWindowProvider.ts 13.68% <0%> (-1.44%) ⬇️
... and 76 more

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 ffd0eb0...f6a9b10. Read the comment docs.

@karrtikr karrtikr added the no-changelog No news entry required label Jan 7, 2020
Comment thread CONTRIBUTING.md Outdated

### Running System Tests

Note: System tests are those in files with extension `.test.ts` but which are not `.unit.test.ts`.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

They are not necessarily test.ts, it can be test*.ts (For eg. testvirtualenvs.ts)

Suggested change
Note: System tests are those in files with extension `.test.ts` but which are not `.unit.test.ts`.
Note: System tests are those in files with extension `.test*.ts` but which are not `.unit.test.ts`.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Ah right, thanks. Are there any other types which might be worth mentioning explicitly?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

(b0d0fca expands the pattern as suggested)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Ah yes. There is these .functional.test.ts files, which are run just like unit tests, not system ones. That is, you don't need to follow steps mentioned for system tests to run those.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Ah, yeah, I see there's npm run test:functional. Is there some documentation somewhere on what sorts of things those tests cover?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Are you referring to what tests with suffix .functional.test.ts cover?
I don't think we've documentation for those. They're kind of like system tests, but test a smaller block of code.

But they are run just like unit tests. So for purposes of this PR, you can just mention steps to run .functional.test.ts are just like steps to run unit.test.ts.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Apologies for the delay. I've added a small section on functional tests: f6a9b10

Copy link
Copy Markdown

@karrtikr karrtikr left a comment

Choose a reason for hiding this comment

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

LGTM✔

@karrtikr karrtikr closed this Jan 16, 2020
@karrtikr karrtikr reopened this Jan 16, 2020
@sonarqubecloud
Copy link
Copy Markdown

SonarCloud Quality Gate failed.

Bug C 1 Bug
Vulnerability A 0 Vulnerabilities (and Security Hotspot 3 Security Hotspots to review)
Code Smell A 7 Code Smells

No Coverage information No Coverage information
1.2% 1.2% Duplication

@karrtikr karrtikr added this to the 2020 January Sprint milestone Jan 16, 2020
@karrtikr karrtikr merged commit 5f453cd into microsoft:master Jan 16, 2020
@karrtikr karrtikr self-assigned this Jan 22, 2020
@lock lock Bot locked as resolved and limited conversation to collaborators Jan 30, 2020
@PeterJCLaw PeterJCLaw deleted the clarify-unit-tests-docs branch June 20, 2020 11:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

no-changelog No news entry required

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants