Closed Bug 1313688 Opened 8 years ago Closed 7 years ago

Fail a test if stubs don't match stubs created by stub generators

Categories

(DevTools :: Console, defect, P2)

defect

Tracking

(firefox54 fixed)

RESOLVED FIXED
Firefox 54
Tracking Status
firefox54 --- fixed

People

(Reporter: linclark, Assigned: nchevobbe)

References

Details

Attachments

(4 files)

Currently, the stub generators don't run on CI. We just use them when we know that packets or stubs would have changed.

However, this means that stubs could go stale, and then our unit tests wouldn't catch possible issues when the backend changes.

We should set up the tests to run in CI. When the generated packet or stub is different from the one saved in the file, the test would fail with a message that the developer should update the stub files. When they do that, the unit tests will run with the new stubs in CI (Bug 1312823) and fail if it was a breaking change. This has the side benefit of giving us a clear record of all API changes.
Depends on: 1307905
Priority: -- → P2
Summary: Make stub generators run as tests on CI → Fail a test if stubs don't match stubs created by stub generators
Flags: qe-verify-
Whiteboard: [new-console]
Assignee: nobody → chevobbe.nicolas
Status: NEW → ASSIGNED
Iteration: --- → 54.1 - Feb 6
Priority: P2 → P1
Iteration: 54.1 - Feb 6 → 54.2 - Feb 20
Iteration: 54.2 - Feb 20 → ---
Priority: P1 → P2
okay, so here's a try run which contains the tests, and edits to the code that would change the stubs. If everything goes well, all the check_* tests should fail.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=44207742bcbf728148f34411768f49fd5a20c0e9
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #1)
> okay, so here's a try run which contains the tests, and edits to the code
> that would change the stubs. If everything goes well, all the check_* tests
> should fail.
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=44207742bcbf728148f34411768f49fd5a20c0e9

There were some problems with this, let's try https://treeherder.mozilla.org/#/jobs?repo=try&revision=668d8f1019af11f5255b347517d9233dc6cf1cf9
Okay, now this seems good.
This is the try run with my patch : https://treeherder.mozilla.org/#/jobs?repo=try&revision=b218e5850a5f069effe5494e3f25459223fcd0c4 , which show no errors.
And here's the one where I made a change which impact the stubs, without updating them : https://treeherder.mozilla.org/#/jobs?repo=try&revision=a35dc336c4c0156df67d0db88c00d195fb8a7cab

We can see that the tests fail as expected (e.g. https://treeherder.mozilla.org/#/jobs?repo=try&revision=a35dc336c4c0156df67d0db88c00d195fb8a7cab&selectedJob=78204968 ).

I'll push my patch to review soon
Comment on attachment 8838492 [details]
Bug 1313688 - Move new console stub generation to head.js;

https://reviewboard.mozilla.org/r/111846/#review114994

::: devtools/client/webconsole/new-console-output/test/fixtures/stub-generators/browser.ini:12
(Diff revision 1)
>    test-network-event.html
> -  test-tempfile.css
> -  test-tempfile.js
>  
>  [browser_webconsole_update_stubs_console_api.js]
>  skip-if=true # This is only used to update stubs. It is not an actual test.

not relevant to this changeset, but you should be able to set skip-if = true on the whole test suite instead of disabling each test individually.

::: devtools/client/webconsole/new-console-output/test/fixtures/stub-generators/head.js:88
(Diff revision 1)
>          res.message.arguments.forEach((argument, i) => {
> +          let existingArgument = existingPacket.message.arguments[i];
>            if (argument && argument.actor) {
> -            argument.actor = existingPacket.message.arguments[i].actor;
> +            argument.actor = existingArgument.actor;
> +          }
> +          if (argument && argument.class === "Window") {

Maybe a comment here explaining why we do this?

::: devtools/client/webconsole/new-console-output/test/fixtures/stub-generators/head.js:228
(Diff revision 1)
>  };
>  `;
>  }
> +
> +function* generateConsoleApiStubs() {
> +  const { consoleApi: snippets } = require("devtools/client/webconsole/new-console-output/test/fixtures/stub-generators/stub-snippets.js");

Maybe we could have a single require for the whole file here?
Attachment #8838492 - Flags: review?(jdescottes) → review+
Comment on attachment 8838492 [details]
Bug 1313688 - Move new console stub generation to head.js;

https://reviewboard.mozilla.org/r/111846/#review114994

> Maybe we could have a single require for the whole file here?

Do you mean moving `const { consoleApi, pageError, ...} = require("devtools/client/webconsole/new-console-output/test/fixtures/stub-generators/stub-snippets.js");` outside of the `generateConsoleApiStubs` function (and the others alike) ?
Comment on attachment 8838492 [details]
Bug 1313688 - Move new console stub generation to head.js;

https://reviewboard.mozilla.org/r/111846/#review114994

> not relevant to this changeset, but you should be able to set skip-if = true on the whole test suite instead of disabling each test individually.

I think we will still need to disable them one by one since we add the check tests here (in another commit)
Comment on attachment 8838493 [details]
Bug 1313688 - Stubs generation;

https://reviewboard.mozilla.org/r/111848/#review115032
Attachment #8838493 - Flags: review?(jdescottes) → review+
Comment on attachment 8838494 [details]
Bug 1313688 - Fix mocha tests.

https://reviewboard.mozilla.org/r/113426/#review115060
Attachment #8838494 - Flags: review?(jdescottes) → review+
Comment on attachment 8838495 [details]
Bug 1313688 - Fail a test if stubs don't match stubs created by stub generators;

https://reviewboard.mozilla.org/r/113428/#review115064

Great improvement, thanks!
Attachment #8838495 - Flags: review?(jdescottes) → review+
Pushed by chevobbe.nicolas@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/2add96f767a5
Move new console stub generation to head.js; r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/a0e7769a42f3
Stubs generation; r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/99ef9fe1494c
Fix mocha tests. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/0d7b8fb5255e
Fail a test if stubs don't match stubs created by stub generators; r=jdescottes
Whiteboard: [new-console]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.