Skip to content

Fix panic if a WithStats suite skips a test early. #1723

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

FGasper
Copy link

@FGasper FGasper commented Apr 2, 2025

Summary

This fixes issue #1722.

Changes

Prevent stats.end() if stats.start() never ran.

Motivation

Allow skipping a test in a setup/before hook. See issue #1722 for an example.

Related issues

Closes #1722

@ccoVeille
Copy link
Collaborator

ccoVeille commented Apr 3, 2025

While it works, I would have done differently to focus the change on the small portion of code that causes the issues.

Here is the code that fails

func (s SuiteInformation) end(testName string, passed bool) {
        s.TestStats[testName].End = time.Now()
        s.TestStats[testName].Passed = passed
}

Your PR is about avoiding to call end, so to avoid the panic. But end can still panic.
A future refactoring may call this method again, and then a panic might occur again

I would like to suggest limiting the change in your PR to this

func (s SuiteInformation) end(testName string, passed bool) {
        if _, started := s.TestStats[testName]; !started {
          return
        }
        
        s.TestStats[testName].End = time.Now()
        s.TestStats[testName].Passed = passed
}

The test you added is good.

@FGasper
Copy link
Author

FGasper commented Apr 3, 2025

@ccoVeille I myself think it’s better, if start() was never called, to avoid calling end(); thus, I’d rather the fix be as I have it.

It’s not something I feel strongly about, though, so I’ll make the change you requested.

@FGasper
Copy link
Author

FGasper commented Apr 3, 2025

@ccoVeille Altered accordingly, with a small tweak to reduce repetition.

Copy link
Collaborator

@ccoVeille ccoVeille left a comment

Choose a reason for hiding this comment

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

@ccoVeille I myself think it’s better, if start() was never called, to avoid calling end(); thus, I’d rather the fix be as I have it.

I understand your point, thanks for considering my fix.

Good idea with that tweak

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.

skip in SetupTest() with HandleStats() causes panic
2 participants