8295424: adjust timeout for another JLI GetObjectSizeIntrinsicsTest.java subtest #11278
Conversation
|
/issue JDK-8295424 |
|
/label add hotspot-runtime |
|
👋 Welcome back dcubed! A progress list of the required criteria for merging this PR into |
openjdk
bot
changed the title
|
@dcubed-ojdk The primary solved issue for a PR is set through the PR title. Since the current title does not contain an issue reference, it will now be updated. |
|
@dcubed-ojdk |
|
@dcubed-ojdk |
|
@dcubed-ojdk |
|
@dcubed-ojdk |
|
/label add tools |
|
@dcubed-ojdk |
|
@dcubed-ojdk
|
|
/label add javadoc |
|
@dcubed-ojdk |
| @@ -66,12 +69,18 @@ | |||
| import toolbox.JavacTask; | |||
| import toolbox.ToolBox; | |||
|
|
|||
| import jdk.test.lib.Platform; | |||
| import jtreg.SkippedException; | |||
There was a problem hiding this comment.
Nit: the order of imports on 72-73 needs to be swapped.
There was a problem hiding this comment.
Why? 'jdk' comes before 'jtreg' and 'Platform' comes before 'SkippedException'.
What am I missing here?
There was a problem hiding this comment.
Mild grumble: langtools tests do not rely on jdk test libraries
There was a problem hiding this comment.
Does langtools have its own test libraries that I can use to ask the same questions?
There was a problem hiding this comment.
Sorry, I was not clear.
The Fuzz.java has this order:
+import jdk.test.lib.Platform;
+import jtreg.SkippedException;
I thought, you ordered imports by names. Then it is better to keep this order unified.
It is really minor though.
There was a problem hiding this comment.
Sorry I'm still confused. As far as I can see, I've added the imports the
same way in both Fuzz.java and TestRedirectLinks.java.
And the imports are in sort order:
'jdk' comes before 'jtreg' and 'Platform' comes before 'SkippedException'.
There was a problem hiding this comment.
Sorry, copied fragment from a wrong file.
This file has imports out of order:
test/langtools/jdk/javadoc/doclet/testLinkOption/TestRedirectLinks.java
+ * @build jtreg.SkippedException
+ * @build jdk.test.lib.Platform
|
@dcubed-ojdk This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 131 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
|
@sspitsyn - Thanks for the review! |
There was a problem hiding this comment.
I accept the javadoc change but dislike the general methodology: it's too much like brushing the dirt under the carpet.
In general, I think it is better to use keywords, or @require to mark such tests and then (if using keywords) use command-line options to filter out such tests.
|
@jonathan-gibbons - Thanks for the review! I could not find an @requires incantation for saying do-not-use-slowdebug-bits
Please see the parent bugs for JDK-8297367 |
Something like:
|
|
Do you plan on closing the CRs associated with these changes even though the root causes are not being addressed, just avoided? It's not clear what is meant by "Test is unstable". Is the test buggy, or are these JVM issues? In either case shouldn't we be trying to understand why it is unstable with slowdebug bug not fastdebug? |
| @@ -84,6 +87,10 @@ public class Fuzz implements Runnable { | |||
| static final Path TEST_DIR = Path.of(System.getProperty("test.src", ".")); | |||
|
|
|||
| public static void main(String[] args) { | |||
| if (Platform.isSlowDebugBuild() && Platform.isOSX() && Platform.isAArch64()) { | |||
There was a problem hiding this comment.
I don't like the idea of skipping the unstable test using SkippedException. Wouldn't be better to add problemlist for slowdebug? So anyone could easy identify test bugs in slowdebug mode. Really it would be better to support bits configurations in standard problem lists like os/arch but it is a separate issue.
There was a problem hiding this comment.
As far as I know, the ProblemList does not support bits config so there's no way
to specify an entry for 'release' or 'fastdebug' or 'slowdebug' or...
There was a problem hiding this comment.
Yes, it is needed to make a separate problem list for this and use it in your testing.
The SkippedException and '@requires' are used to filter out the test when it is not applicable for this configuration, not when there is a bug that reproduced only with this configuration. Adding '@requires' usually means that we are not planning to run.
If you want to add them as exception might be it makes sense to add a corresponding comment.
|
@sspitsyn, @dholmes-ora, @plummercj and @lmesnik - Thanks for the reviews! Sorry for the delay in getting back to this review. I had an over abundance of |
Thanks! I'll test these suggestions! |
These two CRs are like ProblemListing bugs: JDK-8297367: disable TestRedirectLinks.java in slowdebug mode They are both sub-tasks of the bugs that describe the slowdebug failures
I suspect that the tests have inherent assumptions about how long
Yes and that's why the parent bugs will still be open. This is just like ProblemListing a |
|
@jonathan-gibbons, @sspitsyn, @dholmes-ora, @plummercj and @lmesnik - I think I've replied to all of the comments made so far. I still have to checkout Please let me know if these replies are acceptable to you. |
|
The suggested approach is good enough for me. The comments and bugs to fix/investigate slowdebug behaviour are welcome! |
A change like this: results in a complaint from jtreg like this:
both work as does just plain:
so |
|
I also checked for the name |
|
@dholmes-ora - please let me know if you are okay with these fixes since |
|
@dcubed-ojdk no objection from me. I was just offering what I hoped was a solution. But I see now that |
|
My jdk-20+26 stress run #1 includes these patches (as have stress runs for several /integrate |
|
Going to push as commit 6e54705.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
|
@dcubed-ojdk Pushed as commit 6e54705. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Misc stress testing related fixes:
JDK-8295424 adjust timeout for another JLI GetObjectSizeIntrinsicsTest.java subtest
JDK-8297367 disable TestRedirectLinks.java in slowdebug mode
JDK-8297369 disable Fuzz.java in slowdebug mode
Progress
Issues
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/11278/head:pull/11278$ git checkout pull/11278Update a local copy of the PR:
$ git checkout pull/11278$ git pull https://git.openjdk.org/jdk pull/11278/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 11278View PR using the GUI difftool:
$ git pr show -t 11278Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/11278.diff