Skip to content
This repository was archived by the owner on Dec 16, 2023. It is now read-only.

feat: allow to manually remove unused fixtures#156

Open
mweibel wants to merge 2 commits into
assaf:masterfrom
mweibel:remove-unused-fixtures
Open

feat: allow to manually remove unused fixtures#156
mweibel wants to merge 2 commits into
assaf:masterfrom
mweibel:remove-unused-fixtures

Conversation

@mweibel

@mweibel mweibel commented Jul 5, 2018

Copy link
Copy Markdown

This is a work in progress as to ask for comments and also there's no convenient way to remove the fixtures afterwards, only a way to find which matchers haven't been called.
Maybe that's even enough, writing the removal is simple, e.g.:

const unlink = util.promisify(fs.unlink)

afterAll(async () => {
  const unlinkPromises = []
  replay.notMatchedFixtures.map(matcher => {
    unlinkPromises.push(unlink(matcher.fixturePath))
  })
  await Promise.all(unlinkPromises)
  console.log(`Removed ${unlinkPromises.length} unused fixtures.`)
})

This fixes #123

@assaf what's your opinion on this change?
a) would you provide mode or a separate function or nothing at all to actually remove the unused fixtures?
b) what's required to make you merge this PR?

@mweibel mweibel force-pushed the remove-unused-fixtures branch 3 times, most recently from 67ebf65 to b006082 Compare July 5, 2018 14:50
@pstadler

pstadler commented Jul 6, 2018

Copy link
Copy Markdown

This is a very useful feature. I'd suggest to enable it either with something like Replay.unused = 'drop' or env variable e.g. REPLAY_UNUSED=drop, similar to mode.

@mweibel

mweibel commented Oct 26, 2018

Copy link
Copy Markdown
Author

Ping? @assaf

@mweibel

mweibel commented Dec 4, 2018

Copy link
Copy Markdown
Author

ping again.. @djanowski this time, maybe? :)

@djanowski

Copy link
Copy Markdown
Collaborator

Hi @mweibel thanks for submitting a PR!

I'm not too keen on deleting files automatically, especially after some action did not happen. For example, think of accidentally leaving the flag on and running a subset of your test suite...

But we can add reporting for sure.

Maybe report automatically when debugging is on?

Comment thread src/catalog.js Outdated
Comment thread test/replay_test.js
@mweibel

mweibel commented Dec 13, 2018

Copy link
Copy Markdown
Author

I'm not too keen on deleting files automatically, especially after some action did not happen. For example, think of accidentally leaving the flag on and running a subset of your test suite...

Fair point, though hopefully people know what they do when they enable that flag. Also: people usually use version control systems.. If something gets deleted accidentally it can always be retrieved back.
But yeah, I think we can also do just reporting and write in the README how deletion could be added.
Or we add it ourselves but behind another flag?

@mweibel mweibel force-pushed the remove-unused-fixtures branch from b006082 to 62fbf8e Compare December 13, 2018 07:56
@mweibel

mweibel commented Jan 4, 2019

Copy link
Copy Markdown
Author

I added now a README entry with the example code for removing fixtures.

@mweibel mweibel changed the title WIP: feat: allow to manually remove unused fixtures feat: allow to manually remove unused fixtures Jan 8, 2019
@mweibel

mweibel commented Feb 4, 2019

Copy link
Copy Markdown
Author

ping @djanowski

@mehdibeldjilali

Copy link
Copy Markdown

ping @djanowski 😄

@mehdibeldjilali

mehdibeldjilali commented Feb 15, 2019

Copy link
Copy Markdown

A function like that is very helpful... I have Node-Replay with a GraphQL API with a lot of mocks and it's a headcache to remove unused mocks ...

+1 for you @mweibel ! 👍

@mweibel

mweibel commented Apr 3, 2019

Copy link
Copy Markdown
Author

@assaf @djanowski is there any chance this is going to get merged in? I see there are conflicts now because of a recent PR merge. I don't want to put more work into this if you won't merge it.

@mehdibeldjilali

Copy link
Copy Markdown

Hello,

@assaf @djanowski can you think about this feature please ? I've test the fork and it's ok for me. It's the only missing thing of this package.

Thanks !

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Is it possible to include a method/mode to remove unused fixtures?

4 participants