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

Handle tests marked as inconclusive #2405

Merged
merged 19 commits into from
Apr 10, 2024

Conversation

csandfeld
Copy link
Contributor

@csandfeld csandfeld commented Nov 20, 2023

PR Summary

  • Adds properties to CSharp classes to enable Inconclusive and InconclusiveCount properties on the Pester.Run object.
  • Adds logic to group and count tests marked as inconclusive.
  • Pester Report output updated to support Inconclusive counts etc.
  • Adds deprecation notice shown when Set-ItResult -Pending is used in tests.
  • NUnit reports updated to handle inconclusive tests (needs thorough review).
  • Minor changes to help text

Fix #2400

PR Checklist

  • PR has meaningful title
  • Summary describes changes
  • PR is ready to be merged
    • If not, use the arrow next to Create Pull Request to mark it as a draft. PR can be marked Ready for review when it's ready.
  • Tests are added/update (if required)
  • Documentation is updated/added (if required)

Notes

I think the logic to handle inconclusive tests is sound, but I am looking for feedback on these points

  • Are implemented changes to NUnit reports sufficient, or did I miss anything?
  • Does this PR call for new tests or changes to existing ones?
  • Are documentation updates needed (if so, where)?
  • Deprecation notice wording and placement

Copy link
Collaborator

@fflaten fflaten left a comment

Choose a reason for hiding this comment

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

Happy new year! 🎆
Thanks for looking into this. It looks great!

I've left a few comments. There's also a few more places to update:

  • Update example in Set-ItResult comment-based help to include a Inconclusive test + fix summary line which has been wrong the whole time. 🤦‍♂️

  • Update PostPorcess-ExecutedBlock here to check and increase OwnInconlusiveCount similar to skipped. This is used by Inconclusive and OwnInconclusive at Block-level in the result-object ($result.Inconclusive.Block | ft Name, OwnInconclusiveCount, InconclusiveCount). I've suggested a typo fix to related bug in the review.

  • We need some tests to make sure this works and doesn't break in the future. Suggestions:

    • Add test for behavior and error here
    • Extend this test to have a inconclusive test and verify that Inconclusive and InconclusiveCount properties in the result object are correct
    • Add or extend test to make sure inconclusive attribute works as expected in exported NUnit2.5 and NUnit3 reports
  • Include InconclusiveCount in ConvertTo-Pester4Result here

src/functions/Output.ps1 Outdated Show resolved Hide resolved
src/functions/Set-ItResult.ps1 Outdated Show resolved Hide resolved
src/Pester.Runtime.ps1 Show resolved Hide resolved
src/Pester.Runtime.ps1 Outdated Show resolved Hide resolved
@fflaten fflaten added the Feature label Jan 2, 2024
@csandfeld
Copy link
Contributor Author

Happy new year! 🎆 Thanks for looking into this. It looks great!

Thank you @fflaten and late happy new year to you too (sorry about the response time).

I have tried to address your comments and requested changes, but would appreciate some advise on this one though:

  • We need some tests to make sure this works and doesn't break in the future. Suggestions:

    • Add test for behavior and error here

Can I trouble you for an example of what you have in mind that is not already there?

@csandfeld
Copy link
Contributor Author

@fflaten when you have a moment, please let me know if there is anything else I should do or address to complete this PR.

Thanks

Copy link
Collaborator

@fflaten fflaten left a comment

Choose a reason for hiding this comment

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

Getting close! A few more things for the NUnit-reports:

  • Testsuites in NUnit 2.5 will get Success result when it contains a inconclusive test. Need to extend
    }
    if ($InputObject.SkippedCount -gt 0) {
    return 'Ignored'
    }
    if ($InputObject.PendingCount -gt 0) {
    return 'Inconclusive'
    }
    return 'Success'
    }
  • Results in NUnit 3.0 should be Inconclusive if no other tests are failed/passed. Maybe add a PassedCount -gt 0 = Passed elseif and default to Inconclusive?:
    elseif ($InputObject.SkippedCount -gt 0) {
    'Skipped'
    }
    else {
    'Passed'
    }

tst/Pester.RSpec.TestResults.NUnit25.ts.ps1 Outdated Show resolved Hide resolved
tst/Pester.RSpec.TestResults.NUnit25.ts.ps1 Outdated Show resolved Hide resolved
tst/Pester.RSpec.TestResults.NUnit25.ts.ps1 Outdated Show resolved Hide resolved
tst/Pester.RSpec.TestResults.NUnit25.ts.ps1 Outdated Show resolved Hide resolved
tst/Pester.RSpec.TestResults.NUnit3.ts.ps1 Show resolved Hide resolved
tst/Pester.RSpec.TestResults.NUnit3.ts.ps1 Outdated Show resolved Hide resolved
@fflaten
Copy link
Collaborator

fflaten commented Mar 29, 2024

@fflaten when you have a moment, please let me know if there is anything else I should do or address to complete this PR.

Apologies for the delay. Been occupied with work for a while.
We're also waiting on @nohwnd for final review + merge & release.

  • We need some tests to make sure this works and doesn't break in the future. Suggestions:

    • Add test for behavior and error here

Can I trouble you for an example of what you have in mind that is not already there?

Can't remember tbh. so just ignore it. I probably missed the existing inconclusive test 🙂

@nohwnd
Copy link
Member

nohwnd commented Apr 6, 2024

& release. <- Not so easy :D My certificate expired because I asked for renew and then went on vacation. I am waiting for a new one.

@nohwnd nohwnd mentioned this pull request Apr 10, 2024
3 tasks
@nohwnd
Copy link
Member

nohwnd commented Apr 10, 2024

Looking good. I've added cheaper way of checking that the deprecation should be shown so we don't have to inspect every single test on every run.

@nohwnd nohwnd merged commit 3686263 into pester:main Apr 10, 2024
14 checks passed
@nohwnd
Copy link
Member

nohwnd commented Apr 10, 2024

Merged, thank you! :)

@csandfeld
Copy link
Contributor Author

Thank you for accepting the PR @nohwnd. And thank you @fflaten for you patience with me through the code reviews.

@nohwnd
Copy link
Member

nohwnd commented Apr 11, 2024

No thank you for your work, and sorry for being slow! Same huge thanks for Frode 👏

@fflaten
Copy link
Collaborator

fflaten commented Apr 11, 2024

Thank you for accepting the PR @nohwnd. And thank you @fflaten for you patience with me through the code reviews.

Thank you for your patience and great work 👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bring back 'InconclusiveCount' property (or alternative) on returned Pester.Run object
3 participants