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/jetty 12 reorganize session tests #9220

Merged
merged 4 commits into from
Jan 30, 2023
Merged

Fix/jetty 12 reorganize session tests #9220

merged 4 commits into from
Jan 30, 2023

Conversation

sbordet
Copy link
Contributor

@sbordet sbordet commented Jan 27, 2023

No description provided.

@sbordet
Moved jetty-ee9/test-ee9-sessions to jetty-ee9-tests/jetty-ee9-test-s…
7b7cb67 
…essions.

Renamed all child modules to comply with the convention.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@sbordet
Moved jetty-ee10/test-ee10-sessions to jetty-ee10-tests/jetty-ee10-te…
c4aeb56 
…st-sessions.

Renamed all child modules to comply with the convention.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@sbordet
Removed jetty-ee8/test-ee10-sessions since no tests are present for ee8.
a9c43bc 
Furthermore, they should be generated, not be present as sources.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@sbordet
Copy link
Contributor Author

sbordet commented Jan 27, 2023

@janbartel this PR reorganizes the session tests, by putting them under the jetty-eeN-tests parent, and renaming the directories/modules according to the convention we have everywhere else.

However, for ee8 the source files were actually there, rather than being generated from ee9 like all others.
Also in ee8, the jetty-ee8-tests directory is completely missing, so we don't run any of the tests under that directory that we run for ee9.
Given this, I have removed entirely the ee8 session tests given that the run them already for ee9 (like we do for all the others).

Happy to change to what you think it's better.

@joakime joakime added this to the 12.0.x milestone Jan 27, 2023
janbartel
janbartel approved these changes Jan 28, 2023
View reviewed changes
Copy link
Contributor

@janbartel janbartel left a comment

Choose a reason for hiding this comment

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

Just remove the extraneous change with the cdi property and it's good.

Just leaving this comment here, nothing for you to action in this PR: i think in our renaming, prefixing everything with jetty has been overkill - of course it's going to be a jetty artifact, what else would it be :) ?

jetty-ee9/pom.xml Outdated
@@ -25,7 +25,6 @@
<jakarta.annotation.api.version>2.0.0</jakarta.annotation.api.version>
<jakarta.authentication.api.version>2.0.0</jakarta.authentication.api.version>
<jakarta.el.api.version>4.0.0</jakarta.el.api.version>
<jakarta.enterprise.cdi.api.version>3.0.1</jakarta.enterprise.cdi.api.version>
Copy link
Contributor

Choose a reason for hiding this comment

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

Extraneous change.

@olamy
Copy link
Member

olamy commented Jan 28, 2023 •
edited
Loading

might be good to have this as well #9221.
maybe another PR? (grhh a lot merge for me 🤣)
because currently as junit/surefire only reports package name it's impossible in CI to know if failure of org.eclipse.jetty.hazelcast.session.ClusteredOrphanedSessionTest is in ee8 ee9 ee10
currently only gcloud has a good naming.

@sbordet
Restored property removed by mistake.
2e0df21 
Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@sbordet sbordet merged commit 59e55ce into jetty-12.0.x Jan 30, 2023
@sbordet sbordet deleted the fix/jetty-12-reorganize-session-tests branch January 30, 2023 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@janbartel janbartel janbartel approved these changes

@joakime joakime Awaiting requested review from joakime

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
12.0.x
Development

Successfully merging this pull request may close these issues.
4 participants
@sbordet @olamy @janbartel @joakime