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

Docgen: replace assertions for getIntermediateRepresentation unit tests with snapshots #31547

Merged
merged 1 commit into from
May 12, 2021
Merged

Docgen: replace assertions for getIntermediateRepresentation unit tests with snapshots #31547

merged 1 commit into from
May 12, 2021

Conversation

ciampo
Copy link
Contributor

@ciampo ciampo commented May 6, 2021

Description

Replace all the assertions in the getIntermediateRepresentation unit tests with equivalent snapshot checks.

This approach requires less developer maintenance while serving the same purpose as the previous hand-written assertions.

How has this been tested?

  • Run npm run test-unit packages/docgen
  • Make sure that all tests are passing

Screenshots

N/A

Types of changes

Refactor

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • (N/A) I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

@ciampo ciampo requested a review from oandregal as a code owner May 6, 2021 14:09
@sarayourfriend sarayourfriend requested a review from youknowriad May 6, 2021 14:27
@sarayourfriend sarayourfriend added [Tool] Docgen /packages/docgen [Type] Code Quality Issues or PRs that relate to code quality labels May 6, 2021
Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

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

Looks good and tests pass 👍

I might be missing some context or rationale, but I'd love to get a second opinion on whether this is a good (and common in Gutenberg) use case for inline snapshot testing. FWIW I can definitely see value in it and how it reduces the manual part of maintaining these tests, which is why I am inclined to agree to move forward here.

@ciampo
Copy link
Contributor Author

ciampo commented May 12, 2021

I'd love to get a second opinion on whether this is a good (and common in Gutenberg) use case for inline snapshot testing

Absolutely. In fact, we discussed also measuring the time that those tests take before and after this PR — I am planning on posting that data later today

@ciampo
Copy link
Contributor Author

ciampo commented May 12, 2021

I did a little experiment to understand if there's a difference in execution speed when using inline snapshots.

  • I pulled the latest changes on trunk and rebased this PR's branch on top of it
  • I ran the npm run test-unit packages/docgen command for 5 times on both branches

Results

  • Average execution time on trunk: 3.5578s
  • Average execution time on this PR: 3.1494s

Although this is by no means a comprehensive analysis, these first results seem to show that there isn't a loss in performance when running tests with inline snapshots.

View details

On trunk (no snapshots):

Test Suites: 6 passed, 6 total
Tests:       107 passed, 107 total
Snapshots:   0 total
Time:        4.749 s
Ran all test suites matching /packages\/docgen/i.

--- 

Test Suites: 6 passed, 6 total
Tests:       107 passed, 107 total
Snapshots:   0 total
Time:        3.399 s
Ran all test suites matching /packages\/docgen/i.

---

Test Suites: 6 passed, 6 total
Tests:       107 passed, 107 total
Snapshots:   0 total
Time:        4.033 s
Ran all test suites matching /packages\/docgen/i.

---

Test Suites: 6 passed, 6 total
Tests:       107 passed, 107 total
Snapshots:   0 total
Time:        2.818 s
Ran all test suites matching /packages\/docgen/i.

---

Test Suites: 6 passed, 6 total
Tests:       107 passed, 107 total
Snapshots:   0 total
Time:        2.79 s
Ran all test suites matching /packages\/docgen/i.

On this branch (with inline snapshots):

Test Suites: 6 passed, 6 total
Tests:       107 passed, 107 total
Snapshots:   27 passed, 27 total
Time:        3.645 s
Ran all test suites matching /packages\/docgen/i.

--- 

Test Suites: 6 passed, 6 total
Tests:       107 passed, 107 total
Snapshots:   27 passed, 27 total
Time:        3.848 s
Ran all test suites matching /packages\/docgen/i.

---

Test Suites: 6 passed, 6 total
Tests:       107 passed, 107 total
Snapshots:   27 passed, 27 total
Time:        2.72 s
Ran all test suites matching /packages\/docgen/i.

---

Test Suites: 6 passed, 6 total
Tests:       107 passed, 107 total
Snapshots:   27 passed, 27 total
Time:        2.72 s
Ran all test suites matching /packages\/docgen/i.

---

Test Suites: 6 passed, 6 total
Tests:       107 passed, 107 total
Snapshots:   27 passed, 27 total
Time:        2.814 s
Ran all test suites matching /packages\/docgen/i.

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

Nice, thank you for working on this PR 👍🏻

@gziolo gziolo merged commit bc5f0fe into WordPress:trunk May 12, 2021
@github-actions github-actions bot added this to the Gutenberg 10.7 milestone May 12, 2021
@ciampo ciampo deleted the refactor/docgen-tests-use-snapshots branch May 12, 2021 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Tool] Docgen /packages/docgen [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants