-
-
Notifications
You must be signed in to change notification settings - Fork 620
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
[WIP] E2E tests for init and migrate using docker and soren #169
Conversation
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.
I'd use e2e
lowercased everywhere especially for file names but otherwise it looks great to me!
Gotcha, so what's left is to add this for migrate and change to lower case. |
Maybe also fixing the lintin issues reported by codacy/pr? |
Make sure to have installed docker before you continue. | ||
|
||
```sh | ||
$ docker build -t webpack-cli . |
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.
The installation kept on failing on my environment due to the protocol of the dependencies installed via git.
Once I added the +https
it worked:
"yeoman-generator": "git+https://github.com/ev1stensberg/generator.git#Feature-getArgument"
and
"recast": "git+https://github.com/kalcifer/recast.git#bug/allowbreak",
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.
yep, changing those once we're post-yeoman migration to webpack-addons
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.
yep, that's expected
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.
I'd still recommend to add the +https
, so the docker testing works until the migration is complete, we never know for sure when it will happen, so better have a working docker testing until then, for all the users, instead of having people to add the +https
on their own and not commit the changes
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.
Could you post a screenshot of your terminal? Should work regardless
Some errors with migrate, there's still errors, |
sure: @https issue of the package.json about the broken tests: I'd not recommend to merge into master the PR with the broken tests, as it would add up to the "broken window" syndrome, and would also imply that we are ok with having broken tests in the master code, which is not really advisable. |
I dunno what the broken window syndrome is :/ |
Just noticed that the broken tests come from The broken window syndrome recommends that broken tests, poor quality, lintin issues, and such similarities, should be fixed right away. Leaving them would progressively add up, and add up, until the point that are many broken windows to fix and is more expensive to fix them all in one go, as would have been to fix a single broken window when it was found. The Apart from this, is your PR complete? |
In the meeting, we decided to get input from @okonet on how his experience with Ink were, so I'd like a update on that first! |
8ae8a79
to
dc1cab4
Compare
No description provided.