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

Detect dual test case run #824

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

Conversation

std-max
Copy link
Contributor

@std-max std-max commented May 10, 2022

Fix #795

Prevent from calling twice the same test case.
Reset the number of run tests when test_runner_setup is called.
Add tests.

@LarsAsplund
Copy link
Collaborator

The error detection looks good but I don't think we should make a the ability to re-run a test case (by calling test_runner_setup a second time) a new feature. It could be something that works just because of the way our procedures and functions are designed but the proposed error message is advertising it as a feature.

I'm afraid that unorthodox use of VUnit might lead into unforeseen complexities (just as calling a test case twice did) so I rather keep the core of VUnit structure very clean. Did you have a specific use case for this in mind?

@eine eine added this to the v4.7.0 milestone May 11, 2022
@std-max
Copy link
Contributor Author

std-max commented May 21, 2022

@LarsAsplund In fact I added a test that calls test_runner_setup just because it was done in a previous test like the one at line 318 (banner("Should loop over enabled_test_case once and in order unless re-initialized.");
I can remove that of course as I agree with you this might be unusual !

@LarsAsplund
Copy link
Collaborator

@std-max Ok, I understand. I can't really recall if that test was added with a feature in mind or just to make sure that test_runner_setup is well-designed. After all, it is reasonable to expect that a setup works regardless of what happened before it. Still, I wouldn't like to make it a supported feature so if you could remove that part it would be great.

@std-max
Copy link
Contributor Author

std-max commented May 31, 2022

Done @LarsAsplund :)

@std-max
Copy link
Contributor Author

std-max commented Jun 23, 2022

No joke, I literally fell asleep while waiting for the checks to pass
You can now merge @LarsAsplund haha

@eine eine modified the milestones: v4.7.0, v4.8.0, v5.0.0 Apr 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Detect dual test case run
3 participants