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

7539: JDP Tests failing in VPN environments #369

Closed
wants to merge 9 commits into from

Conversation

parttimenerd
Copy link
Contributor

@parttimenerd parttimenerd commented Jan 21, 2022

Only run broadcasting related tests when supported.
Tested on Mac M1 with and without VPN.


Progress

  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JMC-7539: JDP Tests failing in VPN environments

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jmc pull/369/head:pull/369
$ git checkout pull/369

Update a local copy of the PR:
$ git checkout pull/369
$ git pull https://git.openjdk.java.net/jmc pull/369/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 369

View PR using the GUI difftool:
$ git pr show -t 369

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jmc/pull/369.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Jan 21, 2022

👋 Welcome back parttimenerd! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

Copy link
Collaborator

@RealCLanger RealCLanger left a comment

Choose a reason for hiding this comment

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

You also need to update the copyright years.

I'll test this later with my VPN.

@openjdk openjdk bot added the rfr label Jan 21, 2022
@mlbridge
Copy link

mlbridge bot commented Jan 21, 2022

Webrevs

@thegreystone
Copy link
Member

I think it would be better to have the test provide information about how to properly configure the machine to properly run the tests than to automatically skip the tests. Perhaps we could also add a system property check or something to make it easy to skip the test from the command line, if someone truly can't get it to work. But then it should be a conscious choice, not something happening automatically to avoid having to fix the setup.

Here is for example one possible scenario if we go with automated skipping:

  1. Images used for our automated testing change their configuration, no longer providing a route for local multicast.
  2. Tests are automatically skipped.
  3. JDP changes and breaks.
  4. We are never the wiser (until some JDP using person complains post release).

We shouldn't automatically skip tests without a very strong reason. It's better to inform the runner of the test how to fix their config.

@parttimenerd
Copy link
Contributor Author

An option could be to log a warning if we skip a test.

@RealCLanger
Copy link
Collaborator

I think it would be better to have the test provide information about how to properly configure the machine to properly run the tests than to automatically skip the tests. Perhaps we could also add a system property check or something to make it easy to skip the test from the command line, if someone truly can't get it to work. But then it should be a conscious choice, not something happening automatically to avoid having to fix the setup.

Here is for example one possible scenario if we go with automated skipping:

  1. Images used for our automated testing change their configuration, no longer providing a route for local multicast.
  2. Tests are automatically skipped.
  3. JDP changes and breaks.
  4. We are never the wiser (until some JDP using person complains post release).

We shouldn't automatically skip tests without a very strong reason. It's better to inform the runner of the test how to fix their config.

I agree - we should catch the failure and then log something like:
This test failed. Please check if your multicast is working or you are running in a VPN configuration. If this can't be fixed, build with mvn -DskipJDPMulticastTests=true ....

And then check for this property when deciding whether to run the test.

@thegreystone
Copy link
Member

Yup. We can even provide information about the proper command to use on Mac for adding the route.

@openjdk
Copy link

openjdk bot commented Jan 21, 2022

@parttimenerd This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

7539: JDP Tests failing in VPN environments

Reviewed-by: clanger, hirt

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 4 new commits pushed to the master branch:

  • 7037e74: 7541: Switch to using 2021-09 by default
  • 3d03643: 7540: IItemCollectionJsonSerializerTest.java might fail on Windows due to line endings
  • baa6c77: 7537: Build script assumes that Darwin always targets x86_64
  • c646b14: 7477: JMC8.2 does not build with eclipse 4.19 (21-03)

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@thegreystone) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready label Jan 21, 2022
@parttimenerd
Copy link
Contributor Author

My current change still misses the integration in the Windows build script (but this might not be a problem as the VPN stuff primarily seems to affect Mac users) and a short remark in the README. Any thoughts about windows?

Copy link
Collaborator

@RealCLanger RealCLanger left a comment

Choose a reason for hiding this comment

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

Looks good, I only have a few minor suggestions. For me it's ok to leave out the windows build script for now. I can add this later on, I want to play with that script anyway, have just built with maven so far.

@thegreystone thegreystone self-requested a review January 22, 2022 14:25
@thegreystone
Copy link
Member

thegreystone commented Jan 22, 2022

@RealCLanger has provided good insights! Let's act on his feedback before pushing this.

Copy link
Collaborator

@RealCLanger RealCLanger left a comment

Choose a reason for hiding this comment

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

LGTM

@parttimenerd
Copy link
Contributor Author

/integrate

@openjdk openjdk bot added the sponsor label Jan 24, 2022
@openjdk
Copy link

openjdk bot commented Jan 24, 2022

@parttimenerd
Your change (at version e3d853d) is now ready to be sponsored by a Committer.

@thegreystone
Copy link
Member

/sponsor

@openjdk
Copy link

openjdk bot commented Jan 24, 2022

Going to push as commit f3ca1b1.
Since your change was applied there have been 4 commits pushed to the master branch:

  • 7037e74: 7541: Switch to using 2021-09 by default
  • 3d03643: 7540: IItemCollectionJsonSerializerTest.java might fail on Windows due to line endings
  • baa6c77: 7537: Build script assumes that Darwin always targets x86_64
  • c646b14: 7477: JMC8.2 does not build with eclipse 4.19 (21-03)

Your commit was automatically rebased without conflicts.

@openjdk
Copy link

openjdk bot commented Jan 24, 2022

@thegreystone @parttimenerd Pushed as commit f3ca1b1.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

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

Successfully merging this pull request may close these issues.

3 participants