WICKET-7072 wicket tester and core tests without test jar #666

gluser1357 · 2023-09-29T14:58:02Z

Just before the weekend, here is the new PR for the updated branch which seems to run now for Eclipse and VSCode.

See latest comments and test results on WICKET-7072.

Thank you for testing and reviewing in different environments.

Wicket does not use maven-site-plugin since a long time Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

…dule Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

…tests-without-test-jar

bitstorm

I'm generally happy with this PR, it's a good improvement for the JPMS support of Wicket. I think I've found some other JPMS related problems working on this PR but they probably deserve a separate issue. For example, resources from package org.apache.wicket from wicket-core are not visible from external modules, so in a fully JPMS compliant project we might have problems with simply using core components (like Form). I think we should add 'opens' clause to info-module.java, but as I said this should a work for a separate issue.

Forgot to mention WICKET-7072 for Jira

martin-g · 2023-10-03T11:59:11Z

wicket-cdi/src/main/java/module-info.java

@@ -24,6 +24,7 @@
    requires org.apache.wicket.request;
    requires org.apache.wicket.core;
    requires org.slf4j;
+    requires static jakarta.servlet;


This change is not related to this commit.
I reverted it once and everything was working.
Now it is re-introduced silently again.

Please see my latest comment in WICKET-7072 for leaving this change.

Maven Surefire plugin is only for executing the tests via Maven. It does not affect the compilation phase!

I agree, sorry for mixing up compile and surefire plugins.

In addition: the src/test/java don't have module-info.java, i.e. they are not Java modules!

IMO (esp. after reading https://stackoverflow.com/questions/46613214/java-9-maven-junit-does-test-code-need-module-info-java-of-its-own-and-wher), /src/main/java/module-info.java is used for compiling both /src/main/java and /src/test/java, but compilation of test code seems to be different in Eclipse and Maven, leading to different results (Maven compiles test sources, but Eclipse doesn't do in this case). I think more research is needed here. It would be great if Eclipse would follow Maven in this use case. Meanwhile we could keep requires static jakarta.servlet which would prevent Eclipse developers from manually fixing the build path (via exclude module-info.java, see also my comment in WICKET-7072).

Meanwhile we could keep requires static jakarta.servlet which would prevent Eclipse developers from manually fixing the build path

Fully agree on this!
I just want to propose an improvement:

either mention the reason for the change in the commit message

or even better - use a separate commit for it. Mixing it in the same commit with the wicket-util:test-jar one is not good

Fully agree on this!

Ok, great!

I just want to propose an improvement:

Thank you for this proposal. I'm unsure how to do this practically:

revert the change, commit, redo the change with appropriate comment and commit

or

revert the change, commit, wait until PR is merged into master (with commits squashed), open another PR for WICKET-7072, redo the change with appropriate comment and commit?

@martin-g: Looking at some commits in wicket master it looks like that commits of a PR are squashed, and the comments of these commits are merged and used as comment of the squashed commit. Because of this I decided for the first option (reverted the change, commit, redo, added again and commit) and applied it.

solomax · 2023-10-04T09:39:20Z

Just have tested this branch in VSCode

there no errors :))

Hooray! :)

gluser1357 · 2023-10-04T10:35:45Z

Just have tested this branch in VSCode

there no errors :))

Hooray! :)

Great, thanks for testing!

gluser1357 · 2023-10-05T07:42:15Z

wicket-core/src/main/java/org/apache/wicket/protocol/http/mock/MockHttpSession.java

@@ -146,7 +145,8 @@ public ServletContext getServletContext()
 	@Override
 	public void invalidate()
 	{
-		Session session = (Session) attributes.get("wicket:" + BaseWicketTester.TestFilterConfig.class.getName() + ":session");
+		Session session = (Session) attributes.get("wicket:" + "BaseWicketTester.TestFilterConfig" + ":session"); // TODO


Copied from previous PR #659:

wicket-core/src/main/java/org/apache/wicket/protocol/http/mock/MockHttpSession.java

I think this class should be moved to wicket-tester

Ok, we could do that (I also thought about moving the whole _package org/apache/wicket/protocol/http/mock to wicket-tester but org.apache.wicket.core.util.string.ComponentRenderer in /src/main/java makes use of org.apache.wicket.protocol.http.mock.MockServletContext, so that won’t work).

Should we move only MockHttpSession to wicket-tester then? If not, how to rewrite this line in MockHttpSession.invalidate()?

If we want to move the entire package I don't see many alternatives,,,maybe the best option first glance could be move it into wicket-util module (i.e org.apache.wicket.util.protocol.http.mock ). Waiting for better ideas :-)

I don't think that moving the package from wicket-core to wicket-util would have advantages.

My initial assumption was that mocking stuff should be only test scoped, but it currently isn't (as ComponentRenderer shows). IMO we should leave it as it is (possibly requiring a better fix in MockHttpSession.java, see diff) OR move just MockHttpSession.java to wicket-tester?

I think these classes should stay in -core.

Ok, and what about Session session = (Session) attributes.get("wicket:" + BaseWicketTester.TestFilterConfig.class.getName() + ":session");? Where is this relevant?

This seems to be the only open issue of this PR. Could someone please take a closer look at this?

(I just don't see any reference where this attribute "wicket:[class]:session" is set. Related could be AbstractPersistentPageStore.KEY_PREFIX = "wicket:", but I dont see any other ":session" in the code.)

See org.apache.wicket.session.HttpSessionStore#getSessionAttributePrefix

See org.apache.wicket.session.HttpSessionStore#getSessionAttributePrefix

Ok, I see you changed this
originally: ... attributes.get("wicket:" + BaseWicketTester.TestFilterConfig.class.getName() + ":session");
to: ... attributes.get("wicket:org.apache.wicket.util.tester.BaseWicketTester.TestFilterConfig:session");

I wonder if that's correct since TestFilterConfig is an inner class?

A simple alternative would be to save TestFilterConfig as new file and adapt the line to attributes.get("wicket:org.apache.wicket.util.tester.TestFilterConfig:session");

Good catch!

I fixed the bug!
The best would be to use a real class reference instead of hardcoding a String like now.

wicket-core/pom.xml

wicket-util/pom.xml

…ar' of https://github.com/gluser1357/wicket-fork.git into WICKET-7072-wicket-tester-and-core-tests-without-test-jar

martin-g · 2023-10-12T07:42:44Z

I'll take a new look soon!

This module is just tests for wicket-core Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

martin-g · 2023-10-12T11:19:28Z

@gluser1357 Why most of the commits in this PR are by user @sre ? What is your relation ?

martin-g · 2023-10-12T11:28:37Z

org.apache.wicket.core.util.license.ApacheLicenceHeaderTest does nothing in wicket-core-tests module.
Issues:

it does not override before() and licenseHeaders()
even when 1) is fixed it does not check the sources of wicket-core module

Update: Fixed

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

gluser1357 · 2023-10-12T12:25:22Z

@gluser1357 Why most of the commits in this PR are by user @sre ? What is your relation ?

My fault, its both me, sre = gluser1357 (Eclipse switched sometimes back to other author/commit that I use in another repo). Sorry for mixing this up :/

org.apache.wicket.core.util.license.ApacheLicenceHeaderTest does nothing in wicket-core-tests module. Issues:
1. it does not override `before()` and `licenseHeaders()`

2. even when 1) is fixed it does not check the sources of `wicket-core` module
Update: Fixed

Thanks for fixing, I obviously forgot the two overrides in this class.

Are sources of wicket-core checked now, or is something to do here?

sre · 2023-10-12T12:25:48Z

@gluser1357 Why most of the commits in this PR are by user @sre ? What is your relation ?

None of them are from me. You see 'sre', because that has been used as the full name for git when authoring the commits. Github just shows the name of the commit, since there is no account connected to the mapchart.com mail address used by the 'sre' in this pull request.

martin-g · 2023-10-12T12:28:22Z

Are sources of wicket-core checked now, or is something to do here?

ceacaf2#diff-cd87ffc1ff8683a3f9ae0fcc7055de1691b36452d962a438d32b72840d5f0c99R137-R151

martin-g · 2023-10-12T12:29:34Z

Merging ...
More improvements could be made in a separate PR. This one is too big already!

martin-g · 2023-10-12T12:31:58Z

Thanks for pushing this work, @gluser1357 !

gluser1357 · 2023-10-12T12:34:53Z

@gluser1357 Why most of the commits in this PR are by user @sre ? What is your relation ?

None of them are from me. You see 'sre', because that has been used as the full name for git when authoring the commits. Github just shows the name of the commit, since there is no account connected to the mapchart.com mail address used by the 'sre' in this pull request.

@sre: Ah, ok, I see, we are both using "sre" ;-) I really have to take care about that when commiting, sorry for this confusion. I wonder that my "sre" commits haven't been checked and rejected by Github :/

gluser1357 · 2023-10-12T12:54:15Z

Are sources of wicket-core checked now, or is something to do here?

ceacaf2#diff-cd87ffc1ff8683a3f9ae0fcc7055de1691b36452d962a438d32b72840d5f0c99R137-R151

Ok, tests runs for me on wicket-core with this change, nothing to do anymore here.

gluser1357 · 2023-10-12T13:15:51Z

Thanks for pushing this work, @gluser1357 !

No problem. Thank you as well for your great support. 10.0.0-M2 master doesn't show any errors in Eclipse anymore, that's really great 👍

bitstorm · 2023-10-12T14:17:02Z

@gluser1357 thank you indeed for your effort! 🙏🏻

sre and others added 21 commits September 25, 2023 15:00

first commits

9f697d5

adapt parent pom

6e5995b

moved some testers to src/test and adapted poms for mvn install

deb7f35

adapt pom dependency orders and fixed module-info

ce344ce

fixes in pom.xml for parent run

089910a

fixed dependency order in main pom.xml

f3300e4

added wicket-tester (not included yet)

7f58007

follow-up, the big move of wicket-core tests to libs (name clash!)

169fcc5

tweak useModulePath

d19b011

fixes, now building.

c2b529f

added docs in pom.xml

e884abc

WICKET-7072 Remove maven-site-plugin related files

efd9fde

Wicket does not use maven-site-plugin since a long time Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

WICKET-7072 Update end year for Copyright to 2023

d02dbb0

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

WICKET-7072 Remove more maven-site-plugin artifacts

8809ac5

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

WICKET-7072 Cleanup pom.xml files

39be9a9

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

WICKET-7072 Move back the JS tests to wicket-core/src/test/js

d6897c4

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

WICKET-7072 Move WebSocketTester to wicket-native-websocket-tester mo…

a7f2c9e

…dule Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

revert test-jar from wicket-util (see WICKET-7072 26/Sept/23 comment)

5f28ef1

removed one unused dependency

366dad3

added JUnit as non-test dependency in wicket-native-websocket-tester

34e5580

Merge branch 'apache:master' into WICKET-7072-wicket-tester-and-core-…

c3297b7

…tests-without-test-jar

bitstorm approved these changes Oct 2, 2023

View reviewed changes

martin-g requested changes Oct 3, 2023

View reviewed changes

gluser1357 mentioned this pull request Oct 4, 2023

[WICKET-7072] wicket tester and core tests #660

Closed

gluser1357 commented Oct 5, 2023

View reviewed changes

gluser1357 mentioned this pull request Oct 5, 2023

Wicket 7072 wicket core as test jar #659

Closed

solomax reviewed Oct 7, 2023

View reviewed changes

wicket-core/pom.xml Outdated Show resolved Hide resolved

solomax reviewed Oct 7, 2023

View reviewed changes

wicket-util/pom.xml Outdated Show resolved Hide resolved

sre and others added 5 commits October 9, 2023 09:32

removed commented-out code

be28b0e

Merge branch 'WICKET-7072-wicket-tester-and-core-tests-without-test-j…

45d0b94

…ar' of https://github.com/gluser1357/wicket-fork.git into WICKET-7072-wicket-tester-and-core-tests-without-test-jar

removed unused dependency

ba6cad5

reverted requires jakarta.servlet (was not related to test-jar)

5b36edc

added requires jakarta.servlet to support default Eclipse test compile

d77f485

gluser1357 requested a review from martin-g October 11, 2023 06:21

martin-g added 2 commits October 12, 2023 14:06

WICKET-7072: Do not deploy wicket-core-tests artifacts

69af24f

This module is just tests for wicket-core Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

WICKET-7072 Use the fully qualified name of TestFilterConfig class

70a45d0

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

WICKET-7072 Fix the ApacheLicenceHeaderTest for wicket-core

ceacaf2

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

martin-g force-pushed the WICKET-7072-wicket-tester-and-core-tests-without-test-jar branch from 15fcba5 to ceacaf2 Compare October 12, 2023 11:51

martin-g approved these changes Oct 12, 2023

View reviewed changes

martin-g merged commit b0f3da3 into apache:master Oct 12, 2023
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WICKET-7072 wicket tester and core tests without test jar #666

WICKET-7072 wicket tester and core tests without test jar #666

gluser1357 commented Sep 29, 2023

bitstorm left a comment •

edited

martin-g Oct 3, 2023

gluser1357 Oct 4, 2023

gluser1357 Oct 4, 2023

martin-g Oct 4, 2023

gluser1357 Oct 4, 2023

gluser1357 Oct 11, 2023

solomax commented Oct 4, 2023

gluser1357 commented Oct 4, 2023

gluser1357 Oct 5, 2023

bitstorm Oct 5, 2023

gluser1357 Oct 5, 2023

martin-g Oct 5, 2023

gluser1357 Oct 5, 2023

gluser1357 Oct 11, 2023

martin-g Oct 12, 2023

gluser1357 Oct 12, 2023

martin-g Oct 12, 2023

martin-g Oct 12, 2023

martin-g commented Oct 12, 2023

martin-g commented Oct 12, 2023

martin-g commented Oct 12, 2023 •

edited

gluser1357 commented Oct 12, 2023 •

edited

sre commented Oct 12, 2023

martin-g commented Oct 12, 2023

martin-g commented Oct 12, 2023

martin-g commented Oct 12, 2023

gluser1357 commented Oct 12, 2023

gluser1357 commented Oct 12, 2023

gluser1357 commented Oct 12, 2023

bitstorm commented Oct 12, 2023

WICKET-7072 wicket tester and core tests without test jar #666

WICKET-7072 wicket tester and core tests without test jar #666

Conversation

gluser1357 commented Sep 29, 2023

bitstorm left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

solomax commented Oct 4, 2023

gluser1357 commented Oct 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martin-g commented Oct 12, 2023

martin-g commented Oct 12, 2023

martin-g commented Oct 12, 2023 • edited

gluser1357 commented Oct 12, 2023 • edited

sre commented Oct 12, 2023

martin-g commented Oct 12, 2023

martin-g commented Oct 12, 2023

martin-g commented Oct 12, 2023

gluser1357 commented Oct 12, 2023

gluser1357 commented Oct 12, 2023

gluser1357 commented Oct 12, 2023

bitstorm commented Oct 12, 2023

bitstorm left a comment •

edited

martin-g commented Oct 12, 2023 •

edited

gluser1357 commented Oct 12, 2023 •

edited