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

Lighthouse#2273: Stop Fixtures attempting to set static (Map) fields #1281

Merged
merged 9 commits into from
Dec 25, 2018
Merged

Lighthouse#2273: Stop Fixtures attempting to set static (Map) fields #1281

merged 9 commits into from
Dec 25, 2018

Conversation

tazmaniax
Copy link
Collaborator

@tazmaniax tazmaniax commented Nov 15, 2018 •
edited
Loading

Fixes

Lighthouse #2273

Purpose

Stops Fixtures attempting to set static (Map) fields

Background Context

Fixtures probably shouldn't be setting static fields

asolntsev
asolntsev approved these changes Nov 15, 2018
View reviewed changes
Copy link
Contributor

@asolntsev asolntsev left a comment

Choose a reason for hiding this comment

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

@tazmaniax Could you add some test?

@tazmaniax
Copy link
Collaborator Author

@asolntsev not something I've done before but how hard can it be :) I'll be back...

@tazmaniax
Copy link
Collaborator Author

@asolntsev @xael-fry I've run into a class loading issue with my unit test, any idea?

java.lang.RuntimeException: Class models.ClassWithStaticMap was not found [type=models.ClassWithStaticMap,id=instanceA]
	at play.test.Fixtures.loadModels(Fixtures.java:294)
	at play.test.FixturesTest.testStaticMap(FixturesTest.java:57)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:564)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
	at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
	at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57)
	at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
	at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
	at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
	at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27)
	at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
	at org.eclipse.jdt.internal.junit4.runner.JUnit4TestReference.run(JUnit4TestReference.java:86)
	at org.eclipse.jdt.internal.junit.runner.TestExecution.run(TestExecution.java:38)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:538)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:760)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:460)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.main(RemoteTestRunner.java:206)
Caused by: java.lang.ClassNotFoundException: models.ClassWithStaticMap
	at java.base/java.lang.ClassLoader.findClass(ClassLoader.java:709)
	at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:563)
	at play.classloading.ApplicationClassloader.loadClass(ApplicationClassloader.java:99)
	at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:496)
	at play.test.Fixtures.loadModels(Fixtures.java:259)
	... 28 more

@asolntsev
Copy link
Contributor

@tazmaniax Sure. Your class is located in package play.test.models.ClassWithStaticMap, but Play tries to load it from another package: models.ClassWithStaticMap.

Write full class name in FixturesTest.yml:

play.test.models.ClassWithStaticMap(instanceA):
    name: hello

@tazmaniax
Copy link
Collaborator Author

@asolntsev thanks. I was hoping the Play classloader, with the way I'd configured it would have added the test folder as a classpath endpoint so the relative path of "models" would have been picked up but alas no. As it turns out when I did what you suggested Play prefixed the fully qualified class name with "models." so I got another class loading exception...

Caused by: java.lang.ClassNotFoundException: models.play.test.models.ClassWithStaticMap
	at java.base/java.lang.ClassLoader.findClass(ClassLoader.java:709)
	at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:563)
	at play.classloading.ApplicationClassloader.loadClass(ApplicationClassloader.java:99)
	at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:496)
	at play.test.Fixtures.loadModels(Fixtures.java:259)
	... 28 more

Anyway I got it working after jumping through some mocking hoops with application classes and plugins. Let me know if there was a better way and also if my unit test is valid, thx! Otherwise hopefully this can be accepted.

@asolntsev
Copy link
Contributor

@tazmaniax Yes, it's a bad thing that Play adds prefix models. silently.
I would recommend to just move class play.test.models.ClassWithStaticMap to package models. Them you don't need all these tricks with classloading. KISS, my friend :)

@tazmaniax
Copy link
Collaborator Author

tazmaniax commented Nov 22, 2018 •
edited
Loading

@asolntsev ah that's a much better idea. My first impulse was to keep all of the test assets as close together as possible so it was easier to reason about and there was no prior example of the models package being used but I totally agree it's a better solution than my class loading magic. However on the flip side I did get a better understanding of the Play class loading :)

@tazmaniax
Copy link
Collaborator Author

@asolntsev is this good to go?

@asolntsev
Copy link
Contributor

@tazmaniax @xael-fry PR is good. Recommended to merge.

@asolntsev asolntsev self-assigned this Nov 27, 2018
@tazmaniax
Copy link
Collaborator Author

@xael-fry could you merge this? Once this is merged I'm going to build the head of master. thx

@tazmaniax
Copy link
Collaborator Author

@asolntsev @xael-fry can this be merged?

@asolntsev asolntsev merged commit bf22d43 into playframework:master Dec 25, 2018
@asolntsev
Copy link
Contributor

@tazmaniax I squashed 9 commits and merged to master branch. Thank you for the contribution!

@tazmaniax tazmaniax deleted the lighthouse#2273-fixture_static_fields branch December 28, 2018 01:31
@tazmaniax
Copy link
Collaborator Author

@asolntsev thx!

@xael-fry xael-fry added this to the 1.5.3 milestone Jan 2, 2019
@xael-fry xael-fry added the defect label Jan 2, 2019
xabolcs added a commit to xabolcs/play1 that referenced this pull request Jan 4, 2021
@xabolcs
[playframework#1281] Fix porting of pull request playframework#504 fr…
3597eeb 
…om old_master to master

Commit b91659a
("[playframework#878] Add OrderBy support in simplified queries") was never ported
to master.

The changes from Pull Request playframework#504 touched some parts of it,
therefore half of the OrderBy change sneaked in to the "new" master.

Fixing by removing the irrelevant OrderBy change.

In method findByToJPQL in file framework/src/play/db/jpa/JPQL.java:
- old_master merge commit has only StringBuilder related change
- master "merge" commit has this OrderBy change too

old_master merge commit: b6797a1
ported master commit: 97c54d2

Fix: 97c54d2 ("Merge pull request playframework#504 from marcelmay/lighthouse-1502-patch")
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asolntsev asolntsev asolntsev approved these changes

Assignees

@asolntsev asolntsev

Labels
defect
Projects
None yet
Milestone
1.5.3
Development

Successfully merging this pull request may close these issues.
3 participants
@tazmaniax @asolntsev @xael-fry