-
Notifications
You must be signed in to change notification settings - Fork 18
Slice error tests #12
Conversation
Craigacp
left a comment
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.
Looks good apart from the module-info update. We'll fix that another way rather than contaminate the runtime module with things that the tests need.
| */ | ||
| module org.tensorflow.ndarray { | ||
| requires jdk.unsupported; // required by raw buffer implementations using Unsafe | ||
| requires java.desktop; // required for java.awt.* |
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 is only used in a test, and I'd prefer to refactor the test to use a serialized byte array than have java.desktop as a module level dependency for the whole of TF.
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.
Ok, I will remove it.
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 we could also supply a module-info in src/test/java which has the extra requires, that should placate the module system and Maven.
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.
@Craigacp Got a minimal module-info but needed some adjustments. First, if I stick to the file name module-info.java, compilation fails. If I use the file name module-info.test, then it compiles and executes the tests correctly.
The simplest version that I pushed, only adds that one requirement and also changes the module name to org.tensorflow.ndarray.test. This last change is not necessary. Keeping the same name, works. As per this link, I changed the name.
In the link above, the example seem to have the same entries of the main module repeated in the test module. Should that be done?
In addition to this, they have (for JUnit5):
requires transitive org.junit.jupiter.engine;
requires transitive org.junit.jupiter.api;
With these, compilation and testing also work. Want to add these too?
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 different module name implies that we won't easily be able to do white box testing (i.e. access protected methods for testing). If it does have a different module name it should have a dependency on the org.tensorflow.ndarray module, and either way I think it should have the dependencies on junit.
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.
So which is it? Keep the same name or add a dependency?
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 would prefer to use the same name and have Maven patch the module overriding it for the tests. However I've definitely run into issues doing that before, particularly if the test module is depended on elsewhere (which it isn't in this case), so it depends how it goes if you try it. I've not had chance to setup the right environment to test it out locally.
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 have kept the same module name. Compilation and testing execute correctly. I could not get the module-info.java file to work, so its named module-info.test. This reference did not help, but may shed light for anyone trying to change this.
Craigacp
left a comment
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.
LGTM, thanks.
While documenting the API usage for my notes, I had a problem discussed in issue #11. This PR adds the tests I used when the problem was detected.
Besides the tests, I noticed a compilation error that was also corrected by adding a
requiresmodule. Seejava/module-info.javathat has this line: