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

Create smoke test for the three commands: forge, generator-options and test-generators #163

Closed
ms14981 opened this issue Feb 2, 2023 · 5 comments

Comments

@ms14981
Copy link
Contributor

ms14981 commented Feb 2, 2023

Motivation:
Currently it would be easy to break one of the commands. For example, file locations are hardcoded (#158). If they are moved, then one of the commands may fail on a PR without us noticing.

Implementation Details:
Create a GitHub workflow (or add to an existing one) that runs each of the three commands.

  1. ./src/index.js forge https://petstore3.swagger.io/api/v3/openapi.json openapi-forge-csharp returns roughly
Loading generator from 'openapi-forge-csharp'
Validating generator
Loading schema from 'https://petstore3.swagger.io/api/v3/openapi.json'
Validating schema
Iterating over 6 files
No formatter found in C:\dev\open-api-forge\openapi-forge\node_modules\openapi-forge-csharp

---------------------------------------------------

            API generation  SUCCESSFUL

---------------------------------------------------

 Your API has been forged from the fiery furnace:
 8 models have been molded
 13 endpoints have been cast
  1. ./src/index.js test-generators -g openapi-forge-typescript returns roughly
Starting tests for generator openapi-forge-typescript
┌──────────────────────────┬───────────┬────────┬────────┬──────┐
│         (index)          │ scenarios │ failed │ passed │ time │
├──────────────────────────┼───────────┼────────┼────────┼──────┤
│ openapi-forge-typescript │    44     │   0    │   44   │  5   │
└──────────────────────────┴───────────┴────────┴────────┴──────┘
  1. ./src/index.js generator-options openapi-forge-javascript (or ./src/index.js generator-options ../openapi-forge-javascript to mix things up with a relative path) returns roughly:
This generator has a number of additional options which can be supplied when executing the 'forge' command.

Options:
  --generator.moduleFormat <value>  The module format to use for the generated
                                    code. (choices: "commonjs", "esmodule",
                                    default: "commonjs")
@ColinEberhardt
Copy link
Collaborator

I think a smoke test is a great idea 👍

ms14981 added a commit to ms14981/openapi-forge that referenced this issue Feb 10, 2023
ms14981 added a commit to ms14981/openapi-forge that referenced this issue Feb 10, 2023
ms14981 added a commit to ms14981/openapi-forge that referenced this issue Feb 10, 2023
ms14981 added a commit to ms14981/openapi-forge that referenced this issue Feb 10, 2023
ms14981 added a commit to ms14981/openapi-forge that referenced this issue Feb 10, 2023
ms14981 added a commit to ms14981/openapi-forge that referenced this issue Feb 10, 2023
ms14981 added a commit to ms14981/openapi-forge that referenced this issue Feb 10, 2023
ms14981 added a commit to ms14981/openapi-forge that referenced this issue Feb 10, 2023
ms14981 added a commit to ms14981/openapi-forge that referenced this issue Feb 10, 2023
ms14981 added a commit to ms14981/openapi-forge that referenced this issue Feb 10, 2023
ms14981 added a commit to ms14981/openapi-forge that referenced this issue Feb 10, 2023
ms14981 added a commit to ms14981/openapi-forge that referenced this issue Mar 24, 2023
ms14981 added a commit to ms14981/openapi-forge that referenced this issue Mar 27, 2023
ms14981 added a commit to ms14981/openapi-forge that referenced this issue Mar 27, 2023
ms14981 added a commit to ms14981/openapi-forge that referenced this issue Mar 27, 2023
ms14981 added a commit to ms14981/openapi-forge that referenced this issue Mar 27, 2023
ms14981 added a commit to ms14981/openapi-forge that referenced this issue Mar 27, 2023
ms14981 added a commit to ms14981/openapi-forge that referenced this issue Mar 27, 2023
ms14981 added a commit to ms14981/openapi-forge that referenced this issue Mar 27, 2023
ms14981 added a commit to ms14981/openapi-forge that referenced this issue Mar 27, 2023
ms14981 added a commit to ms14981/openapi-forge that referenced this issue Mar 27, 2023
ms14981 added a commit to ms14981/openapi-forge that referenced this issue Mar 27, 2023
ms14981 added a commit to ms14981/openapi-forge that referenced this issue Mar 27, 2023
ms14981 added a commit to ms14981/openapi-forge that referenced this issue Mar 27, 2023
ms14981 added a commit to ms14981/openapi-forge that referenced this issue Mar 27, 2023
ms14981 added a commit to ms14981/openapi-forge that referenced this issue Mar 27, 2023
ms14981 added a commit to ms14981/openapi-forge that referenced this issue Mar 27, 2023
ms14981 added a commit to ms14981/openapi-forge that referenced this issue Mar 27, 2023
ms14981 added a commit to ms14981/openapi-forge that referenced this issue Mar 27, 2023
ms14981 added a commit to ms14981/openapi-forge that referenced this issue Mar 27, 2023
ms14981 added a commit to ms14981/openapi-forge that referenced this issue Mar 27, 2023
ms14981 added a commit to ms14981/openapi-forge that referenced this issue Mar 27, 2023
ms14981 added a commit to ms14981/openapi-forge that referenced this issue Mar 27, 2023
ms14981 added a commit to ms14981/openapi-forge that referenced this issue Mar 27, 2023
ms14981 added a commit to ms14981/openapi-forge that referenced this issue Mar 27, 2023
ms14981 added a commit to ms14981/openapi-forge that referenced this issue Mar 27, 2023
ms14981 added a commit to ms14981/openapi-forge that referenced this issue Mar 27, 2023
ms14981 added a commit to ms14981/openapi-forge that referenced this issue Mar 27, 2023
ms14981 added a commit to ms14981/openapi-forge that referenced this issue Mar 28, 2023
ms14981 added a commit to ms14981/openapi-forge that referenced this issue Mar 28, 2023
ms14981 added a commit to ms14981/openapi-forge that referenced this issue Mar 28, 2023
ms14981 added a commit to ms14981/openapi-forge that referenced this issue Mar 28, 2023
@ms14981
Copy link
Contributor Author

ms14981 commented Mar 31, 2023

Now that we've done some work on this, here are some options:

  1. Have a smoke test that checks that no tests ever fail: Smoke testing #186
  2. Have a smoke test that checks that no more tests are failing than before: Smoke Testing - Alternative #194
  3. Have a smoke test that checks that the output of the command is as expected, but does not verify the number of failing tests. The generators already have automated checks that they work on their PRs.
  4. Have a test that always passes, but posts the test results as a PR comment (e.g. 44/45 tests passed (TS)). Sadly this is not working for us due to a known issue "Resource not accessible by integration" actions/first-interaction#10 (Smoke Test that generates a PR comment #192) so we'd need to write some JS to create the comment (and edit it when a new push is made to the same PR): https://stackoverflow.com/questions/16744069/create-comment-on-pull-request

I recommend option 2, but to move to option 3 or 4 if it proves unreliable with false negatives or false positives.

Additionally, the names of the various GitHub Actions should be changed for clarity - "Test Forge" -> "Forge Unit Tests". We can also remove the "Test" Action because it runs the test-generators command on all generators and outputs to a file, which is unlikely to be ever read. This should be covered by the smoke testing anyway.

@gkocak-scottlogic
Copy link
Contributor

(2) seems more powerful to indicate the status of the current state of the branch by having a direct comparison with the main to detect immediate regressions.

Only down side I can think of is that, it brings additional complexity to the testing framework. Although, the implementation in #194 does some redundant testing by cloning the main and retesting to mark the comparison point, with the issues related to creating comments/artifacts from action runs, I think this redundancy should be okay. Ideally, we would want to create a certain artifact for this test run such as a file with the number of tests passing in it and read that number as the comparison point for the branch to use directly to avoid redundant work. But as indicated, this can wait until the relevant issues are not present.


Regarding (1), isn't this the behaviour we actually expect from the current forge test action (considering #157 is an issue and fixed to indicate any test failure as a fail point) directly?

@ms14981
Copy link
Contributor Author

ms14981 commented Apr 3, 2023

Regarding (1), isn't this the behaviour we actually expect from the current forge test action (considering #157 is an issue and fixed to indicate any test failure as a fail point) directly?

I think the original purpose of that test file was to save results and always "pass", but it seems unlikely that anyone will actually look at those results! So, I've deleted it completely here: #196. My suggestion in (1) is to fail the build if any TS tests fail.

ColinEberhardt pushed a commit that referenced this issue Apr 14, 2023
* chore: add smoke test for generator-options command - stdout echoed (#163)

* chore: generator-options smoke test added (#163)

* chore: wip smoke test-generators (#163)

* chore: write smoke test in JS, all three commands pass

* chore: pass smoke test work to JS instead of Github Actions

* chore: move noisy logs to log file

* chore: log available on step failure, clean up comments

* chore: revert delete of workflow files

* fix: incorrectly removed line

* chore: print installed JS and TS generator versions in smoke test

* chore: pass test-generators if it has >= passes cpw master
@ms14981
Copy link
Contributor Author

ms14981 commented Apr 17, 2023

We've merged in smoke tests (#194). I'm sure this will need incremental work, but for now I'll close and let's see how useful (or not!) the smoke tests are...

@ms14981 ms14981 closed this as completed Apr 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants