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

Fixed Ignore exception issue whereby it was still executing every sta… #1538

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

Conversation

eireglas
Copy link
Contributor

Fixed Ignore exception issue whereby it was still executing every statement in the script table even though it will ignore the result

Example below shows IGNORE_ALL_TESTS exception but what I found was it would still execute the lines below even though it would ignore the result. This fix resolves that issue which also help speed up the overall test
image

…tement in the script table even though it will ignore the result
@eireglas
Copy link
Contributor Author

@fhoeben do you mind reviewing?

@fhoeben
Copy link
Collaborator

fhoeben commented Oct 26, 2024

@eireglas what is the advantage of this way of ignoring all compared to just stopping the test?
I believe your change also changed the behaviour of the exception just to ignore assertions in a single table. Is that indeed the intention? That would be functionality to that is not currently available I believe.

I think we also need some tests to document/check the right things are ignored and not executed (not too little or too much)

@eireglas eireglas force-pushed the Fix_Ignore_Exception_Issue_Whereby_It_Still_Executes_Every_Statement branch from 9afa7a1 to 081ea03 Compare October 29, 2024 16:10
@eireglas
Copy link
Contributor Author

@fhoeben Thanks for the feedback. I've added some tests which would previously have failed but now pass due to my change. Also, I have updated the release notes to better explain the change.
To answer your question with regards the advantage of ignoring all compared to just stopping....by ignoring all, the end result will mean a green, success, rather than show yellow or red for failure. I use fitnesse in different environments and by using this IgnoreAllTests or IgnoreScriptTest functionality I can have a test run certain tables and ignore others depending on the environment, which I find very useful.

@fhoeben
Copy link
Collaborator

fhoeben commented Nov 3, 2024

@eireglas looks good, thanks. One last comment: could you update the test so the difference in behaviour between ignore script and all is explicit. I think they currently both show that the script is stopped/ignored, so we don't see that for ignore all subsequent ones are also ignored, while when just the one script is ignored subsequent ones are executed normally.

@eireglas
Copy link
Contributor Author

eireglas commented Nov 4, 2024

@fhoeben Thanks, the 2 new tests I added were just to make sure that when either of the ignore exceptions are thrown that the statement results in a null i.e. not executed.
With my last commit I've just added an extra line to 2 existing tests in SlimTestSystemTableProcessingTest to show where it explicitly tests the differences between how the ignore exceptions should be handled.

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.

3 participants