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
Create new temporary directory/resource extension #491
Create new temporary directory/resource extension #491
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good and is definitely going into the right direction. Keep at it. 😉 (And don't hesitate to let us know if you don't have the time or interest for it any more. We don't want to put pressure on you completing this.)
buildSrc/build.gradle
Outdated
IssueNavigationLink { | ||
option(name: "issueRegexp", value: '#(\\d+)') | ||
option(name: "linkRegexp", value: 'https://github.com/junit-pioneer/junit-pioneer/issues/$1') | ||
if (xmlFile.exists() && xmlFile.isFile()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You added this check, right? Can you let us know why it was necessary? If we need to change this file, we should so it in a separate change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did indeed. It was necessary because when I imported this project into my IntelliJ IDEA, the file .idea/vcs.xml
was absent for some bizarre reason. So whenever I tried to run any of the Gradle tasks, it threw an exception upon trying to parse the file.
That's a good shout, putting this in a separate change. Given I'm busy IRL, is this something that someone from the core team would be able to handle? If not, then that's fine, as I've just put it in my TODO list.
|
||
@Override | ||
public Path get() throws Exception { | ||
return (tempDir = Files.createTempDirectory(prefix)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this reassign a new tempDir
every time get()
is called? Generally speaking, I'd prefer to have a final field, whose value gets assigned on creation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does. I assumed that Resource::get
will be called at most once by the extension and will never be called by user code.
I drove out this code in a "minimalist" TDD fashion, without thinking much, so thanks for pointing out the flaw! It's now in the TODO list.
// TODO: Check that the parameter is annotated with at least @New or @Shared. | ||
// TODO: Check that the parameter is not annotated with both @New and @Shared. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was already checked in supportsParameter
. We still need to find and interpret the annotations, but we don't need to verify proper use of them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, good shout! I agree. They're now in the TODO list to remove.
ReflectionSupport.newInstance(newResourceAnnotation.value(), (Object[]) newResourceAnnotation.arguments()); | ||
extensionContext | ||
.getStore( | ||
ExtensionContext.Namespace.create(ResourceManagerExtension.class, extensionContext.getUniqueId())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need extensionContext.getUniqueId()
here - each extension context already has its own store.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, okay. I assume something like Namespace.create(ResourceManagerExtension.class)
in a constant like NAMESPACE
will do?
extensionContext | ||
.getStore( | ||
ExtensionContext.Namespace.create(ResourceManagerExtension.class, extensionContext.getUniqueId())) | ||
.put(System.identityHashCode(newResourceAnnotation) + "-resource-factory", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand the need for identityHashCode
as follows:
- the store is a key-value store, so we need a key
- we need to make sure not to accidentally reuse the same key or already-registered resources would get kicked out again
Is that correct? If so, I see one other options besides identityHashCode
:
- use a random
long
- chances of collisions are minuscule
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's correct!
I actually made the same observation a few days ago, as I learned that System::identityHashCode
isn't generally a good way of IDing things because when an object is garbage-collected, its memory location can be reused by a new object.
This may not apply in this case, but I think a long
or AtomicLong
would be best in case my assumption is ever invalidated.
It's already in the TODO list.
extensionContext | ||
.getStore( | ||
ExtensionContext.Namespace.create(ResourceManagerExtension.class, extensionContext.getUniqueId())) | ||
.put(System.identityHashCode(newResourceAnnotation) + "-resource", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above, but one more option:
- ask the resource for a unique name - by default it could return its own
identityHashCode
(or the random long), but certain resources may already be unique (e.g. file path forResource<Path>
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think using a random long
/AtomicLong
is easiest to understand, so unless you have any objections, then that's the method I will try first. :)
.orElse("unknown method"))); | ||
}); | ||
ResourceFactory<?> resourceFactory = | ||
ReflectionSupport.newInstance(newResourceAnnotation.value(), (Object[]) newResourceAnnotation.arguments()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know I said to pass these arguments to the constructor of the resource factory, but now I think that was wrong. I think it should be passed to ResourceFactory::create
. That seems to make more sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, yep, that makes sense. It may make Resource(Factory)
easier to understand. It's in the TODO list now.
return Optional.of(testClass); | ||
return Optional.ofNullable(testClass); | ||
} | ||
|
||
@Override | ||
public Optional<Method> getTestMethod() { | ||
return Optional.of(testMethod); | ||
return Optional.ofNullable(testMethod); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain why these changes are needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nipafx It's so that I can make a TestExtensionContext
that has no test method.
This is so that I can test an (admittedly extremely unlikely) edge case in ResourceManagerExtension
where all hell has broken loose and:
ResourceManagerExtension::supportsParameter
thinks a parameter has@New
, but::resolveParameter
disagrees.- And JUnit 5 itself gives
::resolveParameter
anExtensionContext
with an absentgetTestMethod()
, the contents of which I'm using as part of an exception message.
My thinking was to ensure a helpful exception is thrown even in this case.
But I can see now that the change to TestExtensionContext::getTestClass
is not needed. And regardless, this generally feels convoluted.
So I'm happy to revert this hunk and avoid testing this edge case if you'd like?
(For context, here's the edge case I just talked about.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just pushed a commit that reverts this change and the test for the edge case, as it's so incredibly unlikely that such a convoluted test isn't worth the effort of understanding it.
|
||
public @interface Shared { | ||
|
||
Class<? extends ResourceFactory<?>> value(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think using value
as a name for the attribute makes the most sense when you can expect that it will be the only configured attribute in most use cases (because you can spare the value =
). But since we always need the name
as well, users would always have to write value
, in which case I think we should use a more explicit name. What about factory
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, that makes sense! It's now in the TODO list.
@nipafx Wow, thank you for the detailed review! I'm still very interested in working on this, I'm just held back by IRL things again and I don't know when I'll be next free. I might be able to find time over the next few days to respond to the review. |
@jbduncan Take your time - RL is more important than Pioneer! |
Whilst working on junit-pioneer#491, I saw that buildSrc/build.gradle was failing if the .idea directory existed but .idea/vcs.xml did not, which is what was happened for me with my version of IntelliJ IDEA. This commit makes the build script more lenient by only trying to append issue-related XML tags to .idea/vcs.xml if it exists, rather than relying on the presence of just the .idea directory.
Whilst working on junit-pioneer#491, I saw that buildSrc/build.gradle was failing if the .idea directory existed but .idea/vcs.xml did not, which is what was happened for me with my version of IntelliJ IDEA. This commit makes the build script more lenient by only trying to append issue-related XML tags to .idea/vcs.xml if it exists, rather than relying on the presence of just the .idea directory. junit-pioneer#491 (comment) PR: junit-pioneer#532
9df837f
to
d93e1b2
Compare
FYI, I've rebased all the commits in this PR on top of |
While working on #491, I saw that buildSrc/build.gradle was failing if the .idea directory existed but .idea/vcs.xml did not, which is what was happened for me with my version of IntelliJ IDEA. This commit makes the build script more lenient by only trying to append issue-related XML tags to .idea/vcs.xml if it exists, rather than relying on the presence of just the .idea directory. Related discussion: #491 PR: #532
…s hard to understand. In response to comment junit-pioneer#491 (comment)
Can you give us a short summary of the current status of the draft? |
@Bukama The only things left to do are as follows:
I intend to remove the draft status and open this PR for review when the first point is done. |
@nipafx Thanks for following up on this! I believe all the changes we worked on together are now on GitHub. I saw #141 in my inbox recently, and I agree it looks it would benefit from my work. So I'm spending a bit of time this evening to fix the last few bits I need to do before I can put this PR forward for review. Be on the lookout for further news. 😄 |
I'm ready for the next round of reviews! (Hopefully the final one this time, fingers crossed. 🤞) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found two new TODOs.
src/main/java/org/junitpioneer/jupiter/resource/ResourceExtension.java
Outdated
Show resolved
Hide resolved
src/main/java/org/junitpioneer/jupiter/resource/ResourceExtension.java
Outdated
Show resolved
Hide resolved
One more TODO: All new annotations need a |
Ah, it's just occurred to me that by simplifying the However, this would only ever be a problem if users tried to save shared resources in fields. So as another TODO for myself, I'll add a paragraph to the docs to warn users against this. As a longer term solution, I'll raise an issue later to consider running all tests in a test class sequentially if that class has a lifecycle method injected with a shared resource. |
A warning about not saving resources in fields has just been added in commit 25a412f. |
Would you like me to fix the merge conflict between this branch and the main branch, team? |
I've also raised a new issue for the parallelism problem I mentioned in #491 (comment). |
…ocs and attributes in general
We're so close, I can almost taste it! ✨ |
It happened! 🤯 |
Thank you so much for your infinite patience, @jbduncan! 🙏🏾 We finally made it. |
@nipafx You're very welcome, and thank you and everyone else for all your guidance! What a ride and learning experience it's been. 😊 |
I'll keep an eye out for future bug reports and suggestions with this extension, so that if I have the time, I can address then as soon as possible. |
Proposed commit message:
PR checklist
The following checklist shall help the PR's author, the reviewers and maintainers to ensure the quality of this project.
It is based on our contributors guidelines, especially the "writing code" section.
It shall help to check for completion of the listed points.
If a point does not apply to the given PR's changes, the corresponding entry can be simply marked as done.
Documentation (general)
.adoc
file in thedocs
folder, e.g.docs/report-entries.adoc
.adoc
files)Documentation (new extension)
docs/docs-nav.yml
navigation has an entry for the new extensionpackage-info.java
contains information about the new extensionCode
Contributing
README.md
mentions the new contribution (real name optional)I hereby agree to the terms of the JUnit Pioneer Contributor License Agreement.