This repository has been archived by the owner on Aug 3, 2023. It is now read-only.
Remove the fixtures directory #854
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
EverlastingBugstopper
changed the title
EverlastingBugstopper
changed the base branch from
avery/require-webpack
to
master
…te-to-shareholder-value
…te-to-shareholder-value
ashleymichal
approved these changes
Nov 8, 2019
…te-to-shareholder-value
gabbifish
suggested changes
Nov 8, 2019
| type = "webpack" | ||
| "#}; | ||
| "#, | ||
| ); |
There was a problem hiding this comment.
do we also want to call "cleanup" at the end of every one of these tests? I feel like that would be advisable.
There was a problem hiding this comment.
he added it to the preview helper function: https://github.com/cloudflare/wrangler/pull/854/files#diff-eb986e3abcfb440d6233373e1d67f78dR190
There was a problem hiding this comment.
gotcha! I would prefer for cleanup to explicitly happen as its own call instead of being put in preview(), given that preview() should not have cleanup as a side effect. OR change preview() to preview_and_cleanup().
There was a problem hiding this comment.
fwiw, as part of my test refactor work i will likely update fixtures to conditionally clean up based on whether tests pass, per the test fixture library used by wasm_pack.
|
Hmm, how about a function name that describes file creation, preview, and
deletion then? Lumping all three of those under preview is still too many
side effects for an otherwise very simply named function
…On Fri, Nov 8, 2019 at 3:50 PM Avery Harnish ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In tests/preview.rs
<#854 (comment)>:
> type = "webpack"
- "#};
+ "#,
+ );
Doesn't make sense for me to call cleanup separately I think - preview
will both create the files and clean them up. We don't actually create the
files until we enter the preview function even though we call
fixture.createFile that's just a string in memory
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#854>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AB4YZSPJRQ46XZL4I7EYFSDQSXGI7ANCNFSM4JJ7JCHA>
.
|
|
(Just being nit picky from a maintainability standpoint! I shouldn’t have
to dig for where features live if function names can help make that evident
:)
…On Fri, Nov 8, 2019 at 3:51 PM Gabbi Fisher ***@***.***> wrote:
Hmm, how about a function name that describes file creation, preview, and
deletion then? Lumping all three of those under preview is still too many
side effects for an otherwise very simply named function
On Fri, Nov 8, 2019 at 3:50 PM Avery Harnish ***@***.***>
wrote:
> ***@***.**** commented on this pull request.
> ------------------------------
>
> In tests/preview.rs
> <#854 (comment)>:
>
> > type = "webpack"
> - "#};
> + "#,
> + );
>
> Doesn't make sense for me to call cleanup separately I think - preview
> will both create the files and clean them up. We don't actually create the
> files until we enter the preview function even though we call
> fixture.createFile that's just a string in memory
>
> —
> You are receiving this because your review was requested.
> Reply to this email directly, view it on GitHub
> <#854>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AB4YZSPJRQ46XZL4I7EYFSDQSXGI7ANCNFSM4JJ7JCHA>
> .
>
|
|
@gabbifish makes sense! addressed |
delete-merged-branch
bot
deleted the
avery/clean-up-clean-up-everybody-contribute-to-shareholder-value
branch
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
wave goodbye to the fixtures directory
Wrangler no longer relies on reading fixtures from the file system and instead creates them in the tests themselves.