exportOpenApi: provide configuration parameters via command line #1178

mh182 · 2023-05-17T17:43:12Z

Changes in exportOpenApi: the property openApi.specFile may be provided via the command line with -PopenApi.specFile. This would ease the use case of having more than one OpenAPI specification file, as described in #1167.

The exportOpenApi task did not have a test case yet. So I try to gather feedback about how test case should be structured (see #1107).

Test case structure on the file system:

We use the Spock testing and specification framework to test the behavior of the docToolchain tasks.
The unit tests of a docToolchain task are located in src/test/groovy/scripts/<task_name>.
- Reasoning: Groovy unit tests are by convention located in src/test/groovy. The content of this directory mirrors the structure and location of SUT, the system-under-test (i.e. the docToolchain tasks located in scripts)
The file name providing the test code matches the test class name: <task_name>Spec (starting with Uppercase)
- Reasoning: that's how it is now, so why change it?
The tests are isolated: this means all necessary test code and data (files), except for the SUT itself, are located in its directory. Note: the output generated by the tests are generated within the test directory.
- Reasoning:
  - it facilitates the creation of a task test template fore new tasks (cerateTask)
  - it is easier for developers to understand what the minimal data for the test case is
  - it prevents sharing of test data, which increases the likelihood for brittle tests
  - in case a task is removed, its test case can easily be removed as well
Test configuration file should only contain the minimal necessary information. By default, it should be called config.groovy. The test case can have more than one configuration file (config-*.groovy).
- Reasoning: it's an easy pattern to separate configuration files from test data
The structure on the file system of the test data should match the one in the real project

This PR includes the unit tests with the proposed structure for exportOpenApi.

src/test/groovy/scripts/
└── exportOpenApi
    ├── config.groovy
    ├── config-openApi-missing.groovy
    ├── ExportOpenApiSpec.groovy
    └── src
        ├── petstore-v2.0.yaml
        └── petstore-v3.0.yaml

ExportOpenApiSpec.groovy: the Spock unit tests
config.groovy: this is the configuration files used by most tests
config-openApi-missing.groovy: a configuration file where the openApi configuration is missing
src/petstore-*.yaml: the OpenApi specifications used by the test
the Asciidoc file generated by the tests is created in src/test/groovy/scripts/exportOpenApi/build

netlify · 2023-05-17T17:43:21Z

✅ Deploy Preview for dtc-docs-preview ready!

Name	Link
🔨 Latest commit	`a58d05e`
🔍 Latest deploy log	https://app.netlify.com/sites/dtc-docs-preview/deploys/64672fd8b38fd0000846fe6b
😎 Deploy Preview	https://deploy-preview-1178--dtc-docs-preview.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

scripts/exportOpenApi.gradle

PacoVK · 2023-05-19T06:42:48Z

Since this PR introduces a new file structure for tests to put them under src/test/groovy/scripts i would like to open a discussion here.

Although putting tests under src/test/groovy/scripts folder reflects the current packaging of the scripts it still not following the standard conventions for Gradle projects. I would like to keep the current structure src/test/groovy/doctoolchain but start to move the actual .groovy files under scripts to the standard location, in that case src/main/groovy/doctoolchain --> This way IDEs won't struggle with importing docToolchain (currently manual setup is required to be able to link sources)
A further step is to move some custom script logic in a doLast block (like exportStructurizr) into a seperate .groovy file. This way business logic will be separated from the actual Gradle specific logic and we can easily test in isolation. (an example would be exportToConfluence) The actual .gradle task files will look quite similar then.
Some tasks like exportEA even have inner classes in their .gradle build definition. Stuff like that makes testing in isolation impossible.

TL;DR; My suggestion here is to adhere your basic concept which is good, but rather try to restructure the actual source files than try to adopt the test suite to the existing structure.

PacoVK · 2023-05-19T06:44:40Z

One small remark on your current proposal:

src/petstore-*.yaml: the OpenApi specifications used by the test

I'd like to have them under

resources/petstore-*.yaml: the OpenApi specifications used by the test

because the .yaml files are test resources rather than source

mh182 · 2023-05-19T07:32:17Z

Since this PR introduces a new file structure for tests to put them under src/test/groovy/scripts i would like to open a discussion here.

1. Although putting tests under `src/test/groovy/scripts` folder reflects the current packaging of the scripts it still not following the standard conventions for Gradle projects. I would like to keep the current structure `src/test/groovy/doctoolchain` but start to move the actual `.groovy` files under `scripts` to the standard location, in that case `src/main/groovy/doctoolchain` --> This way IDEs won't struggle with importing docToolchain (currently manual setup is required to be able to link sources)

Good feedback. I do not know Gradle standard conventions. I will move the directory containing the test code into the existing src/test/groovy/doctoolchain. But I stand with the proposal, that each test has its own directory with the resources needed to run the test.

2. A further step is to move some custom script logic in a `doLast` block (like `exportStructurizr`) into a seperate `.groovy` file. This way business logic will be separated from the actual Gradle specific logic and we can easily test in isolation. (an example would be `exportToConfluence`) The actual `.gradle` task files will look quite similar then.

I agree with you. But this is out of scope for this ticket. The reason for this PR is to align us "how do we test tasks". The outcome would be a standard/guideline what minimal tests we want to have for new tasks.

3. Some tasks like `exportEA` even have inner classes in their `.gradle` build definition. Stuff like that makes testing in isolation impossible.

Once again, I agree with you. But I would not refactor the existing code without good test coverage.

TL;DR; My suggestion here is to adhere your basic concept which is good, but rather try to restructure the actual source files than try to adopt the test suite to the existing structure.

We can restructure the source code when we have the test code in place ;-)

mh182 · 2023-05-19T07:42:20Z

One small remark on your current proposal:

src/petstore-*.yaml: the OpenApi specifications used by the test

I'd like to have them under

resources/petstore-*.yaml: the OpenApi specifications used by the test

because the .yaml files are test resources rather than source

I disagree with you on this point.

The main point of a test should be to reflect the structure of the documentation project structure. This means, following our standard project structure, if an OpenAPI specification is located in src/docs/some-spec.yaml I even would go so far as to put the spec in the test into <test-directory>/src/docs.

Having "stuff" in a directory "resources" is the "Gradle/Java" way of thinking, which doesn't necessarily reflect a documentation project directoyr structure.

PacoVK · 2023-05-19T08:39:14Z

The main point of a test should be to reflect the structure of the documentation project structure. This means, following our standard project structure, if an OpenAPI specification is located in src/docs/some-spec.yaml I even would go so far as to put the spec in the test into /src/docs.

Ah, yes makes sense, thanks for pointing that out. I agree if we align to adhere the documentation project directory structure.

PacoVK · 2023-05-19T08:40:34Z

But I would not refactor the existing code without good test coverage

This would just kill the scope of this PR i guess :D But if we align on the things discussed above, we could start incrementally.

PacoVK · 2023-05-19T17:59:15Z

src/test/groovy/scripts/exportOpenApi/src/petstore-v2.0.yaml

is this file still in use somewhere?

At the moment nowhere.

I haven't finished this PR. I'm going to remove it.

mh182 · 2023-05-19T18:37:41Z

scripts/exportOpenApi.gradle

+    // The properties defined by 'GenerateTask' have to be declared in the
+    // configuration phase. Defining those properties in doFirst() throws
+    // exception "propert 'X' is final and cannot be changed any further".
    generatorName = 'asciidoc'
-    outputDir = "${targetDir}/OpenAPI".toString()
    inputSpec = "${docDir}/${specFile}" // plugin is not able to find file if inputPath is defined as '.'
+    outputDir = "${targetDir}/OpenAPI".toString()
+
+    // Define cache is always out-of-date to enforce file generation regardless of input and output file
+    outputs.upToDateWhen { false }
+    outputs.cacheIf { false }


@PacoVK I would appreciate it, if you could take a look into the implementation of this task.

The implementation looks strange. Defining the GenerateTask properties in doFirst() throws exceptions. My Gradle mojo is below average. Maybe there is a better way.

mh182 · 2023-05-19T18:39:50Z

scripts/exportOpenApi.gradle

    if (!specFile) {
-        logger.info("\n---> OpenAPI specification file not found in Config.groovy (https://doctoolchain.github.io/docToolchain/#_exportopenapi)")
+        // If the specFile is not defined skip the rest. Otherwise GenerateTask
+        // is called which fails with a misleading error message "file doesn't exist".
+        // We throw the descriptive "missing property" error message during execution phase.
        return
-    } else {
-        logger.info("Found OpenAPI specification in Config.groovy")
    }


This also looks strange. But I had to add this return statement otherwise GenerateTask would throw an exception and I never reached the code in doFirst() which contains a better error message.

need to investigate a bit, on the first glance it's because you use just a 3rd partie task and the implementation is just offering this behaviour. Maybe there is something to turn off the validation

rdmueller

I am fine with the changes

PacoVK

Since this is already an improvement, i am also fine with that

mh182 added 6 commits May 17, 2023 18:22

test exportOpenApi: add minimal test code

12499ad

exportOpenApi: add reference to gradle plugin

128bfc2

test exportOpenApi: add test if specFile not found

6e5d8d0

test exportOpenApi: improve test if specFile not found

2fe902c

test exportOpenApi: improve test for missing openApi.specFile

8acab8b

test exportOpenApi: cleanup test code

a94dac0

mh182 requested review from rdmueller and PacoVK May 17, 2023 17:43

mh182 commented May 17, 2023

View reviewed changes

scripts/exportOpenApi.gradle Outdated Show resolved Hide resolved

exportOpenApi: fix breaking tests

a58d05e

mh182 changed the title ~~Proposal for a test case template to test docToolchain tasks (closes #1107)~~ Proposal for a test case template to test docToolchain tasks May 19, 2023

mh182 changed the title ~~Proposal for a test case template to test docToolchain tasks~~ exportOpenApi: provide configuration parameters via command line May 19, 2023

mh182 linked an issue May 19, 2023 that may be closed by this pull request

Improve exportOpenApi task for easier integration into master documents #1179

Open

PacoVK reviewed May 19, 2023

View reviewed changes

mh182 commented May 19, 2023

View reviewed changes

rdmueller approved these changes May 23, 2023

View reviewed changes

PacoVK approved these changes May 23, 2023

View reviewed changes

PacoVK mentioned this pull request May 26, 2023

Improve publish to confluence #1189

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

exportOpenApi: provide configuration parameters via command line #1178

exportOpenApi: provide configuration parameters via command line #1178

mh182 commented May 17, 2023 •

edited

netlify bot commented May 17, 2023 •

edited

PacoVK commented May 19, 2023

PacoVK commented May 19, 2023

mh182 commented May 19, 2023

mh182 commented May 19, 2023

PacoVK commented May 19, 2023

PacoVK commented May 19, 2023

PacoVK May 19, 2023

mh182 May 19, 2023

mh182 May 19, 2023

mh182 May 19, 2023

PacoVK May 19, 2023

rdmueller left a comment

PacoVK left a comment

exportOpenApi: provide configuration parameters via command line #1178

Are you sure you want to change the base?

exportOpenApi: provide configuration parameters via command line #1178

Conversation

mh182 commented May 17, 2023 • edited

netlify bot commented May 17, 2023 • edited

✅ Deploy Preview for dtc-docs-preview ready!

PacoVK commented May 19, 2023

PacoVK commented May 19, 2023

mh182 commented May 19, 2023

mh182 commented May 19, 2023

PacoVK commented May 19, 2023

PacoVK commented May 19, 2023

PacoVK May 19, 2023

Choose a reason for hiding this comment

mh182 May 19, 2023

Choose a reason for hiding this comment

mh182 May 19, 2023

Choose a reason for hiding this comment

mh182 May 19, 2023

Choose a reason for hiding this comment

PacoVK May 19, 2023

Choose a reason for hiding this comment

rdmueller left a comment

Choose a reason for hiding this comment

PacoVK left a comment

Choose a reason for hiding this comment

mh182 commented May 17, 2023 •

edited

netlify bot commented May 17, 2023 •

edited