Skip to content

automatically clean up aftereach with optout using VTL_SKIP_AUTO_CLEANUP #80

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

Merged
merged 11 commits into from
Aug 19, 2019
Merged

automatically clean up aftereach with optout using VTL_SKIP_AUTO_CLEANUP #80

merged 11 commits into from
Aug 19, 2019

Conversation

Oluwasetemi
Copy link
Contributor

@Oluwasetemi Oluwasetemi commented Aug 10, 2019

Closes #77

  • Automatically call afterEach(cleanup) if afterEach exists.
  • Add an opt-out mechanism by using VTL_SKIP_AUTO_CLEANUP.
  • Add tests asserting the behavior outlined above.
  • Remove cleanup references from tests and docs.
  • Replace cleanup-after-each with a warning.
  • State that the change is a breaking change.

@codecov
Copy link

codecov bot commented Aug 10, 2019

Codecov Report

Merging #80 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #80   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      1           
  Lines          63     66    +3     
  Branches       10     12    +2     
=====================================
+ Hits           63     66    +3
Impacted Files Coverage Δ
src/vue-testing-library.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 297c1c1...5af5d16. Read the comment docs.

@afontcu afontcu added the BREAKING CHANGE This change will require a major version bump label Aug 11, 2019
@dfcook
Copy link
Collaborator

dfcook commented Aug 13, 2019

This looks good, can you add a couple of tests illustrating the usage of VTL_SKIP_AUTO_CLEANUP?

@Oluwasetemi
Copy link
Contributor Author

I will add the test to it right away

@Oluwasetemi
Copy link
Contributor Author

Can you please take a look at this!

@Oluwasetemi
Copy link
Contributor Author

Done!!!

@afontcu
Copy link
Member

afontcu commented Aug 14, 2019

Hi @Oluwasetemi! This looks awesome, thank you 🙌

I was making sure everything was in place, and came up with an idea.

What if we used toMatchInlineSnapshot() (docs) to generate the snapshots to check against?

// auto-cleanup-skip.js

test('first test render a vue component', () => {
  render({
    template: `<h1>Hello World</h1>`
  })

  expect(document.body.innerHTML).toMatchInlineSnapshot(`
    <div>
      <h1>Hello World</h1>
    </div>
  `)
})

test('no cleanup should have happened, renders the first component still', () => {
  expect(document.body.innerHTML).toMatchInlineSnapshot(`
        <div>
          <h1>Hello World</h1>
        </div>
    `)
})
// auto-cleanup.js

test('render the first component', () => {
  render({
    template: `<h1>Hello World</h1>`
  })

  expect(document.body.innerHTML).toMatchInlineSnapshot(`
                <div>
                  <h1>Hello World</h1>
                </div>
        `)
})

test('cleans up after each test by default', () => {
  expect(document.body.innerHTML).toMatchInlineSnapshot(`""`)
})

What do you think about it? I just wrote expect(document.body.innerHTML).toMatchInlineSnapshot() and let Jest create the initial version. Looks like a good use case for snapshot testing!

@Oluwasetemi
Copy link
Contributor Author

It definitely looks better. This is way better than the previous implementation of the tests.

It will be visible to anyone who checks through the test to understand the cleanup and auto clean up properly.

Copy link
Member

@afontcu afontcu left a comment

Choose a reason for hiding this comment

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

This looks great to me 👌

@Oluwasetemi
Copy link
Contributor Author

How will the docs be updated?

@afontcu
Copy link
Member

afontcu commented Aug 15, 2019

How will the docs be updated?

Docs live in https://github.com/testing-library/testing-library-docs/, so a PR should be created there with the appropriate changes.

@afontcu afontcu requested a review from dfcook August 15, 2019 10:18
Copy link
Collaborator

@dfcook dfcook left a comment

Choose a reason for hiding this comment

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

Agree with @afontcu , great work, thanks for this

@afontcu
Copy link
Member

afontcu commented Aug 17, 2019

Just a small note: we are exploring ways of automating CI/CD (see #85 #86), and I believe this PR is a great way of checking the new workflow. This is why is not merged yet, even though it looks great and all work is done 👍

@afontcu afontcu merged commit 394bde7 into testing-library:master Aug 19, 2019
@afontcu
Copy link
Member

afontcu commented Aug 19, 2019

🎉 This PR is included in version 3.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BREAKING CHANGE This change will require a major version bump released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Automatically cleanup if afterEach is detected
3 participants