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.

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

Initial section on testing #30

Closed
wants to merge 9 commits into from
Closed

Conversation

JesperIRL
Copy link
Member

@JesperIRL JesperIRL commented Oct 16, 2020

It's a start.


Progress

  • Change must not contain extraneous whitespace
  • Change must be properly reviewed

Reviewers

Download

$ git fetch https://git.openjdk.java.net/guide pull/30/head:pull/30
$ git checkout pull/30

@bridgekeeper
Copy link

bridgekeeper bot commented Oct 16, 2020

👋 Welcome back jwilhelm! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr label Oct 16, 2020
@mlbridge
Copy link

mlbridge bot commented Oct 16, 2020

src/index.md Show resolved Hide resolved
src/index.md Show resolved Hide resolved
Copy link
Member

@iignatev iignatev left a comment

Choose a reason for hiding this comment

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

Although I understand the desire to have as much relative information in the guide, I don't think we need to repeat ourselves, most of the information presented here is already available in doc/testing.md and doc/hotspot-unit-tests.md. the former document provides more comprehensive documentation about all available/supported testing in OpenJDK, different modes of test execution; the latter gives an overview of different TEST* macros available in hotspot gtest, defines good tests, provides guidelines on writing such tests, etc.

I think we need to find some middle ground here, so we won't duplicate documentation and at the same time, we won't force people to gather required information from N different places. With that in mind, I think it's important to 1st define the goal of 'testing' section of the Guide.

src/index.md Show resolved Hide resolved
src/index.md Show resolved Hide resolved
src/index.md Show resolved Hide resolved
src/index.md Outdated Show resolved Hide resolved
src/index.md Outdated Show resolved Hide resolved
@mlbridge
Copy link

mlbridge bot commented Oct 16, 2020

Mailing list message from Joe Darcy on guide-dev:

Hi Jesper,

Besides the logistical information, I think it would be helpful to add
some conceptual information about JDK testing.

For example, by default changes should include regression tests, unless
there is a compelling reason not to. (link to list of noreg-foo labels).
A given test should run fairly quickly. Most tests run in much less than
10 seconds on common hardware. A test that runs for minutes and minutes
is unlikely to be suitable for a regression tests, but may be
appropriate for other kinds of testing. The tests should pass reliably
on all platforms the JDK runs on. Watch out for platform-specific
differences such as path separators. If a test uses randomness, include
"@key randomness", etc.

Thanks,

-Joe

On 10/16/2020 9:04 AM, Jesper Wilhelmsson wrote:

@JesperIRL
Copy link
Member Author

Although I understand the desire to have as much relative information in the guide, I don't think we need to repeat ourselves, most of the information presented here is already available in doc/testing.md and doc/hotspot-unit-tests.md. the former document provides more comprehensive documentation about all available/supported testing in OpenJDK, different modes of test execution; the latter gives an overview of different TEST* macros available in hotspot gtest, defines good tests, provides guidelines on writing such tests, etc.

I think we need to find some middle ground here, so we won't duplicate documentation and at the same time, we won't force people to gather required information from N different places. With that in mind, I think it's important to 1st define the goal of 'testing' section of the Guide.

The documents in /doc/ requires prior knowledge to understand. Those aren't written for beginners. The goal for the guide in this case is to provide quick links to more information and serve as a buffer to fill in the gaps for a first time tester.

I agree that we should avoid duplication. For the Developers' Guide I want to have just enough information to get everything up and running. For any additional information there should be links to more detailed documentation.

Personally I have issues with documentation that requires knowledge not only to understand, but also to access. But as the source is now on GitHub and the .md files are reasonably readable through the web interface it's at least possible to link to them. I don't know if search engines will index the JDK source code though so for people googling for documentation we still need some indexed document that refers to these documents.

@JesperIRL
Copy link
Member Author

Besides the logistical information, I think it would be helpful to add
some conceptual information about JDK testing.

Yes, I think this is a good idea. There is some of this in /doc/hotspot-unit-tests.md but it's GTest specific and very HotSpot centric, so we should have something that is more generic and suitable for all. I have updated with a few sentences about this. Let me know if you think this needs to be expanded more or have any other suggestions.

src/index.md Outdated Show resolved Hide resolved
src/index.md Outdated Show resolved Hide resolved
src/index.md Outdated Show resolved Hide resolved
src/index.md Outdated Show resolved Hide resolved
src/index.md Show resolved Hide resolved
src/index.md Show resolved Hide resolved
@iignatev
Copy link
Member

Besides the logistical information, I think it would be helpful to add
some conceptual information about JDK testing.

Yes, I think this is a good idea. There is some of this in /doc/hotspot-unit-tests.md but it's GTest specific and very HotSpot centric, so we should have something more generic and suitable for all. I have updated with a few sentences about this. Let me know if you think this needs to be expanded more or have any other suggestions.

Writing jtreg tests provides generic guidelines about good jtreg tests, but I'd say it's generic enough (as it should be as it's part of JTReg's documentation, not OpenJDK documentation) and doesn't talk about things which are specific to OpenJDK, e.g. keywords, best practices, common libraries, etc. langtools guidelines, on the other hand, provides langtools specific guidelines, some of them are adopted by other test suites, some not. I think it makes sense for us to compose some OpenJDK-centric but generic enough guidelines and put them into either this guide or into /doc/jtreg-tests.md and also have component/area-specific guidelines in /doc/.

@mlbridge
Copy link

mlbridge bot commented Oct 20, 2020

Mailing list message from Igor Ignatyev on guide-dev:

On Oct 19, 2020, at 4:05 PM, Jesper Wilhelmsson <jwilhelm at openjdk.java.net> wrote:

On Mon, 19 Oct 2020 21:28:32 GMT, Igor Ignatyev <iignatyev at openjdk.org> wrote:

Ok. The text has been updated to include this information.

Although you updated Gtest's introduction paragraph to reflect that as of now gtest is for hotspot only, I think it
needs to be said here as well.

The Google Test (GTest) framework is intended for unit testing of the C++ native code.

this statement can also create an inadequate impression that you can't/shouldn't test native code w/ jtreg, this isn't
the case as one can a) test native code thru java API; b) use native test code in jtreg tests.
btw, point b) might deserve a sentence or two in JTreg section of this document.

Please provide a sentence or two about b).

what I meant by b) is to have C/C++ test code which calls functions exported by JDK/JVM (such as `JNI_CreateJavaVM`, `ZIP_Open`, `JAWT_DrawingSurfaceInfo::GetDrawingSurfaceInfo`, `jvmtiEnv::GetAllModules`, etc), have that code compiled into shared library and/or executable; and then use in a jtreg test which has `/native`. A simplest example would be `test/hotspot/jtreg/native_sanity/JniVersion.java` and `libJniVersion.c`.

Or maybe add that as a separate change so that you get proper credit for it
and can become a Committer soon :-)

I'm totally fine w/ being an uncredited contributor, it's not about fame, it's about doing the right :)

@JesperIRL
Copy link
Member Author

what I meant by b) is to have C/C++ test code which calls functions exported by JDK/JVM (such as JNI_CreateJavaVM, ZIP_Open, JAWT_DrawingSurfaceInfo::GetDrawingSurfaceInfo, jvmtiEnv::GetAllModules, etc), have that code compiled into shared library and/or executable; and then use in a jtreg test which has /native. A simplest example would be test/hotspot/jtreg/native_sanity/JniVersion.java and libJniVersion.c.

Ok, I get the high level of what you say here, but I don't have enough detail about how this works to be able to write a comprehensive description about it.

I'm totally fine w/ being an uncredited contributor, it's not about fame, it's about doing the right :)

Right. At some point I would like this project to have a few more Committers/Reviewers though, so it for purely selfish reasons that I want you to be famous ;-)

Copy link
Member

@edvbld edvbld left a comment

Choose a reason for hiding this comment

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

I don't have any particular comments on text itself, but IMO we shouldn't use a third-party markdown rendering service for links in OpenJDK documentation. I suggest to either link directly to HTML content hosted on https://openjdk.java.net or link to the markdown source files in the jdk repository.

src/index.md Outdated Show resolved Hide resolved
src/index.md Outdated Show resolved Hide resolved
Copy link
Member

@jddarcy jddarcy left a comment

Choose a reason for hiding this comment

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

I think the addition of some conceptual testing information is sufficient for now, with the possibility of adding more discussion later.

src/index.md Show resolved Hide resolved
src/index.md Show resolved Hide resolved
@mlbridge
Copy link

mlbridge bot commented Oct 22, 2020

Mailing list message from Lance Andersen on guide-dev:

Understand, and I guess my point was should the use of TEST.properties be covered in the dev guide of just left to the JTREG pages. It would be nice to have best practices flushed out.

Best
Lance
------------------

Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
Lance.Andersen at oracle.com

@iignatev
Copy link
Member

Mailing list message from Lance Andersen on guide-dev:

Understand, and I guess my point was should the use of TEST.properties be covered in the dev guide of just left to the JTREG pages. It would be nice to have best practices flushed out.

Best
Lance

Hi Lance,

I agree that we need to write down the best practices and have them easily available and discoverable, but I don't think that the Guide is the right place for them. I see the Guide as a QuickStart or How To Start Hacking OpenJDK In 5 Minutes (as opposed to The Comprehension OpenJDK Guide: How To Do All Possible Things Right And What Things Not To Do). if we start to add all best practices to the guide, inevitably they will either be superficially covered or we end up with lots of information and overwhelm readers, both cases will result in poor comprehension and I'd argue would undermine the purpose of this document and will cause more confusion.

so I'd prefer not to cover TEST.properties here, instead, I suggest creating a ticket to document common best practices in /doc/jtreg-tests.md in the jdk repo, and list TEST.properties as one of the things we need to document.

Cheers,
-- Igor

@LanceAndersen
Copy link
Collaborator

LanceAndersen commented Oct 22, 2020 via email

@JesperIRL
Copy link
Member Author

JesperIRL commented Oct 22, 2020

I agree that we need to write down the best practices and have them easily available and discoverable, but I don't think that the Guide is the right place for them. I see the Guide as a QuickStart or How To Start Hacking OpenJDK In 5 Minutes (as opposed to The Comprehension OpenJDK Guide: How To Do All Possible Things Right And What Things Not To Do). if we start to add all best practices to the guide, inevitably they will either be superficially covered or we end up with lots of information and overwhelm readers, both cases will result in poor comprehension and I'd argue would undermine the purpose of this document and will cause more confusion.

The Guide is intended to cover best practices so I think this is the best place for such documentation. In my opinion only docs that are tied to a specific version of the source code should be in the /doc/ directory. That is not the place for generic guides and documentation. Erik's comments above shows the problem with accessing that documentation - how do we link to it and how do we make it readable without requiring people to download and build the JDK? People expect to find documentation on-line these days.

Wether it's overwhelming or comprehensible is a matter of layout. Even large amounts of information can be made easily accessible with the right layout. I have planned to add a "Quick links"-box at the top of each section, with the relevant links for expert users who just want to get to the technical documentation. In a similar fashion we can have a "Pro-tip" box with some of the more advanced guidelines that might not be relevant for the beginner. It's not a problem to have a lot of text in the Guide as long as it's easily navigateable - and I'm pretty sure most people these days navigate using the search function in their browser.

The JTReg documentation is documentation for the tool and should not contain OpenJDK specific guidelines. Adding another document seems to me to be a step in the wrong direction. We already have too many different documents and nobody can find any information. We need to consolidate and reduce the number of places, not add more.

@LanceAndersen
Copy link
Collaborator

LanceAndersen commented Oct 22, 2020 via email

@LanceAndersen
Copy link
Collaborator

LanceAndersen commented Oct 22, 2020 via email

@iignatev
Copy link
Member

In my opinion only docs that are tied to a specific version of the source code should be in the /doc/ directory.

Unfortunately, any useful documentation will become tied to a specific version, and the Guide already is: you refer to a specific directory layout, the layouts are different in different versions of JDK, tip vs 8u vs 11u; the list of supported test harnesses is version-dependent; even recommendations and some of the best practices are version-dependent; as the presence and content of /doc; ... thus, according to your logic, the Guide must be in /doc, which, I guess, would make it harder "to find documentation on-line".

Actually, I have a slightly different opinion on the whole hypothesis where people expect to find documentation these days. Although it's true for in general people do that online, the information in this Guide and the other related documentation isn't generic information, it's the information for developers. From my experience, most developers expect to find all the needed documentation within their local workspace, not on some external sources. This isn't to say that hosting the same (or similar) documentation on web-servers is useless, it's just not the 1st place I and, I believe, many others would start their search.

We already have too many different documents and nobody can find any information. We need to consolidate and reduce the number of places, not add more.

that, I totally agree with, but my gut feeling is what the root cause of this problem is the absence of a well-known entry point and poor structure of the documentation. the guide might be such an entry point, or it can be CONTRIBUTING.md or README.md in the repo.

-- Igor

@JesperIRL
Copy link
Member Author

Unfortunately, any useful documentation will become tied to a specific version, and the Guide already is: you refer to a specific directory layout, the layouts are different in different versions of JDK, tip vs 8u vs 11u;

I assume that you refer to "Code Owners" here. Actually that list is the union of all directories from JDK 9 and forward. Obviously I might have missed some directory that existed for some time in between, but that should be considered a bug. The intention is to cover all versions. Due to the large difference I didn't include JDK 8 and earlier. Maybe that's a bug as well, but my guess is that anyone working on JDK 8 these days already know who owns the code.

the list of supported test harnesses is version-dependent; even recommendations and some of the best practices are version-dependent; as the presence and content of /doc;

If there are significant differences that is relevant to a large part of the community I think we should mention these differences. The Guide isn't only for developers of mainline. Please also note that the fact that these differences aren't mentioned today isn't because the Guide is version specific but rather because we are currently writing it and large parts of it is still missing.

... thus, according to your logic, the Guide must be in /doc, which, I guess, would make it harder "to find documentation on-line".

Having the Guide in /doc wouldn't make sense at all even if it was version specific since part of the Guide is trying to explain where to find he source code and how to download it. As you know, this part currently don't describe mainline since it was written in 2012 and hasn't been updated since.

Actually, I have a slightly different opinion on the whole hypothesis where people expect to find documentation these days. Although it's true for in general people do that online, the information in this Guide and the other related documentation isn't generic information, it's the information for developers. From my experience, most developers expect to find all the needed documentation within their local workspace, not on some external sources. This isn't to say that hosting the same (or similar) documentation on web-servers is useless, it's just not the 1st place I and, I believe, many others would start their search.

Ok. I have a different experience and can only speak for myself. In other open source projects I've looked at I've always used regular web search to find documentation, and so far I haven't seen any project where that documentation wasn't available in a nicely formatted on-line document. Maybe that says more about what kinds of projects I've been looking at rather than where projects in general keep their source code.

@iignatev
Copy link
Member

it would appear that when I was talking about version-dependent text, I was mostly talking about this particular section( testing), and most of that was suggested by me. so if there is someone to blame for having version-specific text in the guide, that would be me 😄 in any event, I believe this patch is good enough to be integrated, we can identify and fix/clarify version dependent parts later.

-- Igor

@openjdk
Copy link

openjdk bot commented Oct 28, 2020

@JesperIRL This change now passes all automated pre-integration checks.

After integration, the commit message for the final commit will be:

Initial section on testing

Reviewed-by: iignatyev, darcy, lancea, iris

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 3 new commits pushed to the master branch:

  • 041850a: Added contact info for maillist problems.
  • dde2b7b: Fixed links
  • 901eeba: Add notes on avoiding spurious introduction of labels.

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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 master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Oct 28, 2020
@JesperIRL
Copy link
Member Author

/integrate

@openjdk openjdk bot closed this Oct 28, 2020
@openjdk openjdk bot added integrated and removed ready rfr labels Oct 28, 2020
@openjdk
Copy link

openjdk bot commented Oct 28, 2020

@JesperIRL Since your change was applied there have been 3 commits pushed to the master branch:

  • 041850a: Added contact info for maillist problems.
  • dde2b7b: Fixed links
  • 901eeba: Add notes on avoiding spurious introduction of labels.

Your commit was automatically rebased without conflicts.

Pushed as commit 7030605.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@JesperIRL JesperIRL deleted the testing branch October 28, 2020 03:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

7 participants