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.

Sign up for GitHub

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

Jump to bottom

Fix JS tests failing on master #5031

Merged
merged 3 commits into from Oct 26, 2020
Merged

Fix JS tests failing on master #5031

merged 3 commits into from Oct 26, 2020

Conversation

timja
Copy link
Member

@timja timja commented Oct 25, 2020 •
edited

Apparently JS unit tests didn't cause the build to fail when maven.test.failure.ignore was set see #5031 (comment)

@timja timja added the skip-changelog Should not be shown in the changelog label Oct 25, 2020
@timja timja mentioned this pull request Oct 25, 2020
Merged
10 tasks
@timja timja requested a review from a team October 25, 2020 17:18
oleg-nenashev
oleg-nenashev approved these changes Oct 25, 2020
View reviewed changes
Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

Well, it definitely makes sense to migrate tests to divs

@jglick
Copy link
Member

jglick commented Oct 25, 2020

What test failure? URL?

@jglick jglick requested a review from fqueiruga October 25, 2020 19:46
@jglick
Copy link
Member

jglick commented Oct 25, 2020

See also #5032.

@timja
Copy link
Member Author

timja commented Oct 25, 2020 •
edited

What test failure? URL?

looks like they might not be run in CI, @StefanSpieker raised it in #5006.

cd war && yarn test

Test Suites: 1 failed, 2 passed, 3 total
Tests:       6 failed, 14 passed, 20 total
Snapshots:   0 total
Time:        5.604s
error Command failed with exit code 1.

@StefanSpieker
Copy link
Contributor

It is also in CI build, but the build is not failing. See for example: PR-5030 build log
Just search for the Tests: 6 failed, 14 passed, 20 total

@jglick
Copy link
Member

jglick commented Oct 25, 2020

https://ci.jenkins.io/job/Core/job/jenkins/job/master/2235/execution/node/45/log/?consoleFull shows the failure, but I think because we are passing -Dmaven.test.failure.ignore it is treated as nonfatal? So the question is how to report this in junit-compatible format.

@timja
Copy link
Member Author

timja commented Oct 25, 2020

Yes just found that, looking in to it

@jglick
Copy link
Member

jglick commented Oct 25, 2020

https://github.com/eirslett/frontend-maven-plugin/blob/cdc515af830fb0be5f0a1710c538b7b950fef1f9/frontend-maven-plugin/src/main/java/com/github/eirslett/maven/plugins/frontend/mojo/AbstractFrontendMojo.java#L96-L101

@timja
Copy link
Member Author

timja commented Oct 25, 2020

Also FTR this has tripped us up in a release before, javascript linting was failing during release but not in CI, same cause I assume.

@timja
Copy link
Member Author

timja commented Oct 25, 2020 •
edited

RFE for it:
eirslett/frontend-maven-plugin#679

I guess to fix this we might need to move running it outside of maven.

@jglick
Copy link
Member

jglick commented Oct 25, 2020

eirslett/frontend-maven-plugin#564

Is some file created when tests pass that we can assert the existence of?

@timja
Copy link
Member Author

timja commented Oct 25, 2020

I think we can configure a test reporter

@jglick
Copy link
Member

jglick commented Oct 25, 2020

With warnings-ng you mean? That is one option I guess.

@jglick
Copy link
Member

jglick commented Oct 25, 2020

Search for com.github.eirslett.maven.plugins.frontend.lib.TaskRunnerException: ?

@timja
Copy link
Member Author

timja commented Oct 25, 2020

no no just a standard javascript one that emits junit results, almost done, testing locally.

@timja
Copy link
Member Author

timja commented Oct 25, 2020 •
edited

I've temp reverted the js unit test fix just to see it fail in CI, still need to look into lint errors as well to see if it's the same issue, (appears not but prob needs similar fixes)

@timja
Copy link
Member Author

timja commented Oct 25, 2020

Failed correctly in https://ci.jenkins.io/blue/organizations/jenkins/Core%2Fjenkins/detail/PR-5031/2/tests/

@timja timja requested review from a team October 26, 2020 06:17
@timja timja merged commit 277ce4c into jenkinsci:master Oct 26, 2020
@timja timja deleted the fix-js-tests branch October 26, 2020 07:07
@timja
Copy link
Member Author

timja commented Oct 26, 2020

expediting so that we don't get a failed release tomorrow by forgetting to merge this...

jglick
jglick reviewed Oct 26, 2020
View reviewed changes
Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this!

war/.gitignore
@@ -1,5 +1,6 @@
work
/rebel.xml
junit.xml
Copy link
Member

Choose a reason for hiding this comment

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

Would prefer to place it under target/.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jglick jglick jglick left review comments

@oleg-nenashev oleg-nenashev oleg-nenashev approved these changes

@StefanSpieker StefanSpieker StefanSpieker approved these changes

@res0nance res0nance res0nance approved these changes

@fqueiruga fqueiruga Awaiting requested review from fqueiruga

Assignees
No one assigned
Labels
skip-changelog Should not be shown in the changelog
Projects
None yet
Milestone
No milestone
5 participants
@timja @jglick @StefanSpieker @oleg-nenashev @res0nance