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

chore(deps): bump maven-surefire-plugin from 2.22.2 to 3.0.0 #16427

Conversation

dependabot[bot]
Copy link
Contributor

@dependabot dependabot bot commented on behalf of github Mar 27, 2023

Bumps maven-surefire-plugin from 2.22.2 to 3.0.0.

Release notes

Sourced from maven-surefire-plugin's releases.

3.0.0

🚀 New features and improvements

🐛 Bug Fixes

📝 Documentation updates

👻 Maintenance

3.0.0-M9

🚀 New features and improvements

🐛 Bug Fixes

📦 Dependency updates

  • Bump log4j-core from 2.13.1 to 2.17.1 in /surefire-its/src/test/resources/surefire-1659-stream-corruption (#569) @​dependabot
  • Bump junit from 4.12 to 4.13.1 in /surefire-its/src/test/resources/surefire-2065-junit4 (#611) @​dependabot
  • Bump commons-email from 1.2 to 1.5 in /surefire-its/src/test/resources/classpath-filtering (#551) @​dependabot

3.0.0-M8

💥 Breaking changes

🚀 New features and improvements

... (truncated)

Commits
  • 41b640b [maven-release-plugin] prepare release surefire-3.0.0
  • 1b49cdb [SUREFIRE-2156] Refresh download page
  • 6669b03 [SUREFIRE-2150] Remove duplicate license headers
  • c6ccef3 Don't fast break builds on GitHub
  • 2709f76 [SUREFIRE-2154] Get rid of localRepository from surefire mojo parameter, use ...
  • 052ce53 [SUREFIRE-2150] upgrade to parent pom 39 (#614)
  • 208eae2 Sanitize failIfNoSpecifiedTests prefix in failsafe
  • b8246dc [SUREFIRE-2143] Fix reporting of skipped parameterized test
  • 11bbdc9 [SUREFIRE-2149] Make all ITs run with Maven 3.9.0
  • 1f0c261 remove this file, not sure why this was here...
  • Additional commits viewable in compare view

Dependabot compatibility score

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting @dependabot rebase.


Dependabot commands and options

You can trigger Dependabot actions by commenting on this PR:

  • @dependabot rebase will rebase this PR
  • @dependabot recreate will recreate this PR, overwriting any edits that have been made to it
  • @dependabot merge will merge this PR after your CI passes on it
  • @dependabot squash and merge will squash and merge this PR after your CI passes on it
  • @dependabot cancel merge will cancel a previously requested merge and block automerging
  • @dependabot reopen will reopen this PR if it is closed
  • @dependabot close will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
  • @dependabot ignore this major version will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)
  • @dependabot ignore this minor version will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)
  • @dependabot ignore this dependency will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)

Bumps [maven-surefire-plugin](https://github.com/apache/maven-surefire) from 2.22.2 to 3.0.0.
- [Release notes](https://github.com/apache/maven-surefire/releases)
- [Commits](apache/maven-surefire@surefire-2.22.2...surefire-3.0.0)

---
updated-dependencies:
- dependency-name: org.apache.maven.plugins:maven-surefire-plugin
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <[email protected]>
@dependabot dependabot bot added external dependency java Pull requests that update Java code labels Mar 27, 2023
@github-actions
Copy link

github-actions bot commented Mar 28, 2023

Test Results

   978 files  ±0     978 suites  ±0   1h 17m 43s ⏱️ + 2m 20s
6 184 tests ±0  6 146 ✔️ ±0  38 💤 ±0  0 ±0 
6 433 runs  ±0  6 388 ✔️  - 2  45 💤 +2  0 ±0 

Results for commit 4981282. ± Comparison against base commit 0ab8cc9.

This pull request removes 2 and adds 2 tests. Note that renamed tests count towards both.
com.vaadin.flow.ccdmtest.CCDMTest ‑ com.vaadin.flow.ccdmtest.CCDMTest
com.vaadin.flow.ccdmtest.CCDMTest(production) ‑ com.vaadin.flow.ccdmtest.CCDMTest
com.vaadin.flow.ccdmtest.CCDMTest ‑ Unknown test
com.vaadin.flow.ccdmtest.CCDMTest(production) ‑ Unknown test
This pull request removes 2 skipped tests and adds 2 skipped tests. Note that renamed tests count towards both.
com.vaadin.flow.ccdmtest.CCDMTest ‑ com.vaadin.flow.ccdmtest.CCDMTest
com.vaadin.flow.ccdmtest.CCDMTest(production) ‑ com.vaadin.flow.ccdmtest.CCDMTest
com.vaadin.flow.ccdmtest.CCDMTest ‑ Unknown test
com.vaadin.flow.ccdmtest.CCDMTest(production) ‑ Unknown test

♻️ This comment has been updated with latest results.

@Artur-
Copy link
Member

Artur- commented Apr 3, 2023

This needs to be fixed somehow or then surefire 3.0.0 is somehow broken

Char 0xFFFF out of allowed range, line 65, column 49 (TEST-com.vaadin.flow.server.frontend.TaskGenerateTsConfigTest.xml, line 65)

@mcollovati
Copy link
Collaborator

mcollovati commented Apr 3, 2023

Char 0xFFFF out of allowed range

This is because the test is writing an empty tsconfig.json file, and the Json parsers throws an exception as if the file contains the above control character, that is not legal in XML.

@Artur- Since the test only checks that tsconfig.json is not overwritten if existing, do you think it is ok to write a space character as file content? This way the parsing still fails, the test passes, but there are no illegal characters in junit XML test report file

@Artur-
Copy link
Member

Artur- commented Apr 3, 2023

Sounds perfectly fine

@Artur-
Copy link
Member

Artur- commented Apr 3, 2023

Slow tests still has issues

 There was an error in the forked process
  [SUREFIRE] std/in stream corrupted
  java.io.IOException: Resource temporarily unavailable

Sounds a like the failsafe 3.0 issue..

@mcollovati
Copy link
Collaborator

Thanks. I'll take a look

@mcollovati
Copy link
Collaborator

Ok, so this is a known issue
#15102 (comment)
https://issues.apache.org/jira/browse/SUREFIRE-2127

Basically, TaskRunNpmInstall is marked as @NotThreadSafe so the surefire plugin executes it in a forked process.

Using PIPE instead of INHERIT with ProcessBuilder in TaskRunNpmInstall seems to resolve the issue, but I'm not completely sure that this will not raise some other issue

builder.redirectInput(ProcessBuilder.Redirect.PIPE);
builder.redirectError(ProcessBuilder.Redirect.PIPE);

BTW, a similar issue with stdio redirection errors was reported previously for Payara and Vaadin 23 with webpack
#13655

@Artur-
Copy link
Member

Artur- commented Apr 3, 2023

Oh it was in surefire and not failsafe.. I created that issue

@Artur-
Copy link
Member

Artur- commented Apr 3, 2023

What is the difference between PIPE and INHERIT? It sounds like inherit would share the streams and pipe send output to input. If so, it does not sound too problematic to change

@Artur-
Copy link
Member

Artur- commented Apr 3, 2023

@mcollovati
Copy link
Collaborator

Just a side note: The test case in https://github.com/vaadin/flow/blob/upgrade-test-runners/flow-server/src/test/java/com/vaadin/flow/server/frontend/TaskRunPnpmInstallTest.java is not marked as @NotThreadSafe

Right. I was looking at TaskRunNpmInstallTest

@sonarqubecloud
Copy link

sonarqubecloud bot commented Apr 3, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 4 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@mshabarov mshabarov requested a review from mcollovati April 3, 2023 11:28
@mcollovati
Copy link
Collaborator

So the problem is the opposite npm install task works fine because it is started in a forked process, whereas pnpm fails because it runs in the plugin thread.
For this PR, I marked also the pnpm test as @NotThreadSafe
I'll draft another PR changing the process builder to use PIPE redirect

@Artur-
Copy link
Member

Artur- commented Apr 3, 2023

Makes sense. Should we merge this now, as it works?

@mcollovati
Copy link
Collaborator

Yes, I think we can merge this

@Artur- Artur- merged commit f3fc6df into main Apr 3, 2023
@Artur- Artur- deleted the dependabot/maven/org.apache.maven.plugins-maven-surefire-plugin-3.0.0 branch April 3, 2023 12:08
@mcollovati
Copy link
Collaborator

Seems like there are still issues on teamcity. The problem seems to happen when the process writes to STDERR.
Reading the stream when the process gets executed seems to fix the issue.
#16522 and #16520 seem to pass the validation

@Artur-
Copy link
Member

Artur- commented Apr 4, 2023

Maybe the same PRs also fix #16450?

@mcollovati
Copy link
Collaborator

It is probable. Let me try to bump failsafe in the draft PR and let's see what happens

@mcollovati
Copy link
Collaborator

It seems the PR fixes also the failsafe errors.
I cleaned up #16522 to use PIPE, to fix the surefire and failsafe issues so that builds can pass.
After that one, we can still take a look at #16520 to resolve the other issues with stream redirection

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
external dependency java Pull requests that update Java code +0.0.1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants