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.
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
RFC: User Config file integration #53
RFC: User Config file integration #53
Changes from all commits
76f9943
5bfd5d7
25a921f
88c95c1
654a80d
14e1f25
d349358
590ced0
bb7e22d
5d3f8c0
b53fe73
3053247
3cd95cb
7528cea
cd810fe
12133d3
1891719
10151c9
d7ead99
42fbc3a
9fce093
1a0c400
69d9ca4
7e195d8
0a542ac
ebb30ad
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably not for this issue, but maybe we should create an issue to change init.js to context.js?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I agree with that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
#57
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if the intent is to also test with a nested directory output, lets add it to the
describe
description, otherwise I think we can drop those additional tests for nested pages since they are already covered above, and if our architecture is sound, anything that breaks nested directories here should in theory also break it up there, which is a better logical block to solve it in.In essence, we can try and have tests that take advantage of other tests already testing for specific outputs, so tests can have minimal overlap while still breaking each other if something goes wrong when making a change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They aren't covered under this scenario, thats why they are there. In theory they should break earlier in the tests, but not necessarily with all the tests.
Every single situation the public directory, js bundle, hello, nested, need to be tested.
When the source directory changes(which it does as per this test when a config file is copied), everything has to be retested or at least partially.
For example, had we tested the index route in the nested pages describe here(using user workspace mock-app), we would have caught the error for #54 which I just found today because one of the duplicate files which was in the correct path wasn't serialized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right good point.
And I think that's a good approach to take here, as for changing something like the
userWorkspace
, essentially we would want to run everything from the beginning.One thing that isn't so clear here though, and maybe this could just be made more clear in a
describe
orit
is the specific configuration used and making sure toassert
on those specific values somehow?I would expect to see specific tests to cover each config API being supported (
userWorkspace
,devServer
), even if it means recreating a new config file or amending dynamically in some way to make it as explicit as possible, since we should make sure that if the user provides supported configuration itemsThat we call those out specifically in our testing somehow, like though
initConfig
build
Although some cases may have some overlap, I think it's OK if it means that we can define and test the critical paths and outputs of a given feature / workflow / etc for the project if it ensures our tests can be as faithful to how a user would also setup / run our code. Providing a great and accurate development / testing environment we'll provide us much more grounded and confident in the usefulness and coverage of our tests.
A little duplication for the sake of clarity and setup is a positive tradeoff / investment in my book 👍