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

Don't error on old test reports #348

Merged
merged 1 commit into from
Feb 2, 2022
Merged

Conversation

timja
Copy link
Member

@timja timja commented Feb 2, 2022

This is an outdated feature that doesn't reflect modern build tools that won't rebuild if all the inputs are the same (and they aren't forced to rebuild)

A multitude of hacks for gradle that are no longer needed after this:
https://stackoverflow.com/questions/13879667/how-to-fix-test-reports-were-found-but-none-of-them-are-new-did-tests-run-in

Seems like there was no test coverage for this and I don't think it makes sense to add any for a non feature

Fixes #201

@timja timja added the bug label Feb 2, 2022
@timja timja enabled auto-merge (squash) February 2, 2022 08:47
@timja timja merged commit e294b1b into jenkinsci:master Feb 2, 2022
@timja timja deleted the old-test-reports branch February 14, 2022 16:46
@raul-arabaolaza
Copy link

@timja I believe this change broke https://github.com/jenkinsci/maven-plugin/blob/0f600837dbea3d8a86984f34f9cc7eb2e24950da/src/test/java/hudson/maven/reporters/SurefireArchiverUnitTest.java#L175 which was created due to https://issues.jenkins.io/browse/JENKINS-31524 which I believe is still a valid concern.

My knowledge on the internals here is zero so I believe you are the best one to evaluate if JENKINS-31524 is still a concern or is no more relevant

@timja
Copy link
Member Author

timja commented Mar 17, 2022

Hi

I've had a bit of a look and can't say I really understand the maven plugin change that was done.
I would've thought the change here would only help in the reported bug case but not the easiest to understand.

The safest way would be for the user to use separate directories anyway.

@raul-arabaolaza
Copy link

Thanks @timja I am on the same situation, will dig deeper.

@jtnord
Copy link
Member

jtnord commented Mar 18, 2022

But if nothing is rebuilt, and hence the tests are not run, (and the file is outdated) why would we want to report the tests as run / passing in the UI when they are not run?

Because the tests are not run how do you know they pass. For example a test that fails if the date > 19-03-2022 would pass today.
If gradle or whatever never re team that test as the production and test code never changes we would report it tomorrow as passing. (And it would not have passed if ran).
This leads to the issue that people would see a failing test in thier PR, (when compared to the last build on the mainline branch) which with good reason they could assume was due to thier PR.

Iirc (unable to check now) but this issue reported was always fixable if the user wanted it by setting a system property?

The issue we also have now IIUC is that we will also report tests that no longer exist.
E.g do not clean target/ before running tests and remove a Test file?

@timja
Copy link
Member Author

timja commented Mar 21, 2022

This wasn't fixable by a system property.

This fundamentally did not work with gradle's test results previously without work arounds.

This change means that the plugin reports test results that are there. If you don't want them to be reported then you need to not have them there.

Because the tests are not run how do you know they pass. For example a test that fails if the date > 19-03-2022 would pass today.

In the case of gradle the tests will run again as soon as the source set cache is invalided, i.e. if you change one of the tests or the source code. It will not run tests over and over again by just clicking build though, unless you pass some command line args which you can do.

@jtnord
Copy link
Member

jtnord commented Mar 22, 2022

This fundamentally did not work with gradle's test results previously without work arounds.

does it work now with maven (mvn test) for example then rm -fr src/test/java/myTest (the surefire result for test MyTest will now remain and be constantly reported until you wipe out the workspace).

This change means that the plugin reports test results that are there. If you don't want them to be reported then you need to not have them there.

And that is the issue - the plugin should report tests that ran which was the behaviour prior to this change.

I doubt I can be convinced otherwise - likely as I will no doubt fail to convince the gradle users otherwise. Which then means that the only conclusion here is that this needs to be an option.

jtnord added a commit to jtnord/junit-plugin that referenced this pull request Mar 22, 2022
@jglick
Copy link
Member

jglick commented Apr 28, 2022

This appears to leave behind an unused parameter buildTime as well as an obsolete comment

* Collect reports from the given {@link DirectoryScanner}, while
* filtering out all files that were created before the given time.
IIUC?

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.

Allow old test reports
4 participants