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

total skipped and times details added #177

Merged
merged 8 commits into from
Jul 25, 2022

Conversation

a-chatterjee
Copy link
Contributor

@a-chatterjee a-chatterjee commented Jul 22, 2022

Junit report also shows skipped tests and time details for each test suite. The total of skipped tests or time taken are not being populated at the testsuites level.

all testing tools which generate a combined junit report by default, have the combined detail at the root testsuites level. Added them as part of attributes.

@bhovhannes
Copy link
Owner

Hi, can you please provide more details in MR description?

Also, I'll close and reopen MR to see why checks do not run.

@bhovhannes bhovhannes closed this Jul 22, 2022
@bhovhannes bhovhannes reopened this Jul 22, 2022
@bhovhannes
Copy link
Owner

Please add a new test case validating that the total number of skipped tests is correct. Existing test case have it at 0, which does not really check how merging should work.

And I have a question about total time. I don't know how other tools work. Do they somehow take into account that tests may be running in parallel? Maybe adding all times up will not make sense?

@a-chatterjee
Copy link
Contributor Author

Please add a new test case validating that the total number of skipped tests is correct. Existing test case have it at 0, which does not really check how merging should work.

Adding a new test will require adding new test xml files and also a new expected merged report. Since the skipped count is just an extra tag that shows up in testsuite and testsuites whenever there is a test skipped, I have updated the existing fixtures and test case.

Let me know if that is not enough.
.
.
.

And I have a question about total time. I don't know how other tools work. Do they somehow take into account that tests may be running in parallel? Maybe adding all times up will not make sense?

That is right. Just adding up the time wont make sense if it runs in parallel. I will remove that change

@codecov
Copy link

codecov bot commented Jul 23, 2022

Codecov Report

Merging #177 (3bceaeb) into main (bbf5615) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #177   +/-   ##
=======================================
  Coverage   98.30%   98.30%           
=======================================
  Files           4        4           
  Lines          59       59           
=======================================
  Hits           58       58           
  Misses          1        1           
Impacted Files Coverage Δ
src/mergeToString.js 100.00% <ø> (ø)

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 bbf5615...3bceaeb. Read the comment docs.

@bhovhannes bhovhannes merged commit 5a072fd into bhovhannes:main Jul 25, 2022
@bhovhannes
Copy link
Owner

@a-chatterjee , thank you for your work on this!

Your changes have been included in v3.0.6

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.

2 participants