[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR: JDK-8215568: Refactor SA clhsdb tests to use ClhsdbLauncher
From:       JC Beyler <jcbeyler () google ! com>
Date:       2018-12-19 18:12:18
Message-ID: CAF9BGBz9SmM3nt9gekVNRUhmomtZ3YAHb-_vr6DZoCAc2y9DEg () mail ! gmail ! com
[Download RAW message or body]

Hi Jini,

I saw this potential issue with one of the test conversions:
http://cr.openjdk.java.net/~jgeorge/8215568/webrev.00/test/hotspot/jtreg/serviceability/sa/TestPrintMdo.java.udiff.html
                
   - It seems like there is a missing "unexpected" strings check here no?
      - The original test was doing
-
-                if (line.contains("missing reason for ")) {
-                    unexpected = new RuntimeException("Unexpected msg:
missing reason for ");
-                    break;
-                }

whereas the new test is not seemingly (though
http://cr.openjdk.java.net/~jgeorge/8215568/webrev.00/test/hotspot/jtreg/serviceability/sa/TestClhsdbJstackLock.java.udiff.html
 does do it so I think this is an oversight?).

   - Also interesting is that the original test was trying to find one of X:
-                if (line.contains("VirtualCallData")  ||
-                    line.contains("CounterData")      ||
-                    line.contains("ReceiverTypeData")) {
-                    knownProfileDataTypeFound = true;
-                }

whereas you are now wanting to find all of them. Is that normal/wanted?

The rest looked good to me, though I wish there were a way to not have to
change all the strings to be regex friendly but I fail to see how to do
that without writing a runCmdWithoutRegexMatcher, which seems overkill :-)
Jc

On Tue, Dec 18, 2018 at 8:55 PM Jini George <jini.george@oracle.com> wrote:

> Hello!
> 
> Requesting reviews for:
> 
> http://cr.openjdk.java.net/~jgeorge/8215568/webrev.00/
> 
> BugID: https://bugs.openjdk.java.net/browse/JDK-8215568
> 
> No new failures with the SA tests (hs-tiers 1-5) with these changes. The
> changes here include:
> 
> * Refactoring the SA tests which test clhsdb commands to use
> ClhsdbLauncher for uniformity and ease of maintainence.
> * Testing for regular expressions with shouldMatch rather than
> shouldContain.
> * Minor cleanups.
> 
> Thank you,
> Jini.
> 
> 
> 

-- 

Thanks,
Jc


[Attachment #3 (text/html)]

<div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div \
dir="ltr">Hi Jini,<div><br></div><div>I saw this potential issue with one of the test \
conversions:</div><div><a \
href="http://cr.openjdk.java.net/~jgeorge/8215568/webrev.00/test/hotspot/jtreg/service \
ability/sa/TestPrintMdo.java.udiff.html">http://cr.openjdk.java.net/~jgeorge/8215568/w \
ebrev.00/test/hotspot/jtreg/serviceability/sa/TestPrintMdo.java.udiff.html</a><br></div><div> \
- It seems like there is a missing &quot;unexpected&quot; strings check here \
no?<br></div><div>         - The original test was doing  \
</div><div><div>-<br></div></div><div><div>-                        if \
(line.contains(&quot;missing reason for &quot;)) {</div><div>-                        \
unexpected = new RuntimeException(&quot;Unexpected msg: missing reason for \
&quot;);</div><div>-                              break;</div><div>-                  \
}</div></div><div><br></div><div>whereas the new test is not seemingly (though <a \
href="http://cr.openjdk.java.net/~jgeorge/8215568/webrev.00/test/hotspot/jtreg/service \
ability/sa/TestClhsdbJstackLock.java.udiff.html">http://cr.openjdk.java.net/~jgeorge/8 \
215568/webrev.00/test/hotspot/jtreg/serviceability/sa/TestClhsdbJstackLock.java.udiff.html</a> \
does do it so I think this is an oversight?).</div><div><br></div><div>     - Also \
interesting is that the original test was trying to find one of X:</div><div><div>-   \
if (line.contains(&quot;VirtualCallData&quot;)   ||</div><div>-                       \
line.contains(&quot;CounterData&quot;)         ||</div><div>-                         \
line.contains(&quot;ReceiverTypeData&quot;)) {</div><div>-                            \
knownProfileDataTypeFound = true;</div><div>-                        \
}</div></div><div><br></div><div>whereas you are now wanting to find all of them. Is \
that normal/wanted?</div><div><br></div><div>The rest looked good to me, though I \
wish there were a way to not have to change all the strings to be regex friendly but \
I fail to see how to do that without writing a runCmdWithoutRegexMatcher, which seems \
overkill :-)</div><div>Jc</div></div></div></div></div></div></div><br><div \
class="gmail_quote"><div dir="ltr">On Tue, Dec 18, 2018 at 8:55 PM Jini George &lt;<a \
href="mailto:jini.george@oracle.com">jini.george@oracle.com</a>&gt; \
wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px \
0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hello!<br> <br>
Requesting reviews for:<br>
<br>
<a href="http://cr.openjdk.java.net/~jgeorge/8215568/webrev.00/" rel="noreferrer" \
target="_blank">http://cr.openjdk.java.net/~jgeorge/8215568/webrev.00/</a><br> <br>
BugID: <a href="https://bugs.openjdk.java.net/browse/JDK-8215568" rel="noreferrer" \
target="_blank">https://bugs.openjdk.java.net/browse/JDK-8215568</a><br> <br>
No new failures with the SA tests (hs-tiers 1-5) with these changes. The <br>
changes here include:<br>
<br>
* Refactoring the SA tests which test clhsdb commands to use <br>
ClhsdbLauncher for uniformity and ease of maintainence.<br>
* Testing for regular expressions with shouldMatch rather than <br>
shouldContain.<br>
* Minor cleanups.<br>
<br>
Thank you,<br>
Jini.<br>
<br>
<br>
</blockquote></div><br clear="all"><div><br></div>-- <br><div dir="ltr" \
class="gmail_signature"><div \
dir="ltr"><div><br></div>Thanks,<div>Jc</div></div></div>



[prev in list] [next in list] [prev in thread] [next in thread]