-
Notifications
You must be signed in to change notification settings - Fork 14
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
Red test filter #96
Red test filter #96
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cooollll!
I left two important (i guess) commments:
- One is a refactor to improve the code
- The other could be a bug... I had problems (I'm not 100% sure but to investigate) with tests with
expectedFailure
pragma... And maybe it's related with the comment about the use ofunexpectedFailureCount
instead offailures
.
src/MuTalk-Model/MTAnalysis.class.st
Outdated
(testFilter species ~= MTRedTestFilter and: [ | ||
self resultsContainUnexpectedFailuresOrErrorsFor: results ]) | ||
ifTrue: [ MTTestsWithErrorsException signal ]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we avoid the type check using polymorphism?
I propose:
To add a new method: testFilter validateFailures: results
.
It should do:
(self resultsContainUnexpectedFailuresOrErrorsFor: results)
ifTrue: [ MTTestsWithErrorsException signal ]
in the superclass and nothing in MTRedTestFilter
.
Then this method should
MTAnalysis >> initialTestRun [
| results |
results := testCases collect: [ :aTestCase | aTestCase run ].
testFilter validateFailures: results. "Throws error or not based on filters"
testCases := testFilter filterTests: testCases
]
Another option is to throw the exception in the filterTests:
, but I prefer the other option, it's cleaner and easier to implement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree here, the class comparison is not nice :).
If tomorrow we subclass it or add a class with similar behavior, this code will change many many times.
Better to delegate to the filter as @PalumboN says.
About the two options, doing it inside filterTests:
or validateFailures:
, I'm ok with any.
Now looking at the code I'm wondering, does it do what it should? :D
The thing is:
- if I have 10 tests out of which 1 is red
- with the red test filter we should have 9 tests for the analysis, and 1 test ignored
Which means that:
- the filter cannot be just on the "test references" but on the test + result => I see that the tests have a
lastResult
, so this is ok! - If, after running the filters, (and whatever the filter we have) there were broken tests, we should throw the exception => This means we can get the code of the error outside the filter, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anyways, I see you already implemented it, I think my proposal could be an enhancement for later ^^. I propose we open an issue for it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understood exactly what you meant, but if what I did is ok I guess I can fix the merge so we can integrate the pr, and we can discuss about your idea tomorrow @guillep ?
I tried a merge, let's see the test run |
@DurieuxPol there is a test failure, I don't know if it's because I did wrong the merge or not, could you give me a hand here? :) |
yes |
Fixes #28
Added
MTRedTestFilter
, a test filter that allows to run the analysis even if there are failing tests during the initial test run.Refactored by the way:
MTTestCaseReference
to include the last result as instance variable and the#run
methods by moving theMTTestsWithErrorsException
(when there are failing tests in the initial test run) management inMTAnalysis