On Mon, 30 Sep 2024 18:53:57 GMT, Nizar Benalla <[email protected]> wrote:
> Please review this change that adds a new test root `docs` dedicated to
> testing the documentation, which has been a work in progress for a while.
> Tests for links, encoding, HTML, accessibility will be later added in
> following PRs.
>
> We also define a new make target `test-docs` meant for local use and depends
> on the docs.
> This also adds the necessary configurations needed at Oracle.
>
> This patch includes a test `TestDocs` which serves to show developers how
> they are meant to resolve the docs to test them, I want to include it
> temporarily until better tests are added later.
>
> TIA
make/conf/jib-profiles.js line 1082:
> 1080: ],
> 1081: make_args: testOnlyMake,
> 1082: environment: {
I'm missing an environment variable communicating the location of the docs
image here. Something like `DOCS_IMAGE_DIR` should be set so RunTests.gmk can
pick it up and propagate it to jtreg.
make/conf/jib-profiles.js line 1095:
> 1093: if (!testedProfile.endsWith("-jcov")) {
> 1094:
> testOnlyProfilesPrebuiltDocs["run-test-prebuilt-docs"]["dependencies"].push(testedProfile
> + ".jdk_symbols");
> 1095: }
I don't think we need this. Running these tests with jcov doesn't seem to make
sense to me.
make/conf/jib-profiles.js line 1113:
> 1111: profiles = concatObjects(profiles, testOnlyProfilesPrebuiltDocs);
> 1112:
> 1113: if (!new java.io.File(__DIR__, "../../README.md").exists()) {
There are several conditionals and blocks that are duplicated from the
run-tests-prebuilt definitions above. I think I would prefer if you merged this
into the originals instead of duplicating things. I think it's more likely that
future changes will need to update all prebuilt profiles and that will be
easier to get right without duplication.
test/docs/jdk/javadoc/TestDocs.java line 59:
> 57: throw new SkippedException("docs folder not found in either
> location");
> 58: }
> 59: }
This shouldn't be needed. We should define an environment variable that is
expected to point to the docs image that RunTests.gmk sets up. Perhaps there
could be java code to automatically fallback to looking in the first candidate
location here, as that's the layout of the build itself. Some people prefer to
bypass the makefiles and run jtreg standalone, and that would help support that
usecase. I would put that java code in a common library class for the whole
test root.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/21272#discussion_r1782726396
PR Review Comment: https://git.openjdk.org/jdk/pull/21272#discussion_r1782723483
PR Review Comment: https://git.openjdk.org/jdk/pull/21272#discussion_r1782792422
PR Review Comment: https://git.openjdk.org/jdk/pull/21272#discussion_r1782736704
