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

TAP5-2749: Incorrect behavior of getIfExists in EntityApplicationStatePersistenceStrategy #36

Merged
merged 5 commits into from Apr 23, 2023

Conversation

bvfalcon
Copy link
Contributor

https://issues.apache.org/jira/browse/TAP5-2749 -- fix with controlling integration test for this bug

Copy link
Contributor

@vjlamp vjlamp left a comment

Choose a reason for hiding this comment

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

Thank you for contributing this PR. I like your solution.

Thank you also for making the effort of creating a test case, even a whole new test app (7). However, in my opinion, making a small addition to the existing test app 6 would be nicer. Mainly because it would be clearer what the test was about (i.e. making sure getIfExists works properly). Secondly, keeping track of which test app is for what gets more difficult as the number of test apps increase.

Have a look at the SSOEntity page of app 6. Adding something like

@Inject
ApplicationStateManager manager;

public User getUserIfExists()
{
	return manager.getIfExists(User.class);
}

and something like

<p>userIfExists: <span id="userIfExists">${userIfExists}</span></p>

in the page template (to make sure the method gets invoked).

Hopefully you agree this is better for the reasons given above?

@bvfalcon
Copy link
Contributor Author

making a small addition to the existing test app 6 would be nicer.

In case of all tests in app6 are independent from each other this is absolutely true. But tests JpaIntegrationTestWithAnnotationsInServiceImplementation.persist_entities and SessionApplicationStatePersistenceStrategyTest.check collidieren one to another: first sets session variable, second checks this variable on starting. Adding deleteAllVisibleCookies(); in the start of second test solves this issue.

@vjlamp
Copy link
Contributor

vjlamp commented Apr 23, 2023

Hello Vladimir, thank you for picking up my suggestions. I realized only tonight that it's possible for me to commit to PRs. So that's what I did, simplifying the test a little bit more.

I have the entire Tapestry tests ensemble running locally at the moment (just one more time). When all tests pass, I'll merge the PR.

@vjlamp vjlamp merged commit dca4fd0 into apache:master Apr 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants