Skip to content
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

Check the exit code from npm processes #28

Merged
merged 1 commit into from
May 22, 2020

Conversation

alexdutton
Copy link
Member

Without this, npm errors pass silently, potentially leading to 'successful' incomplete webpack builds.

@ppanero
Copy link
Member

ppanero commented May 18, 2020

@alexsdutton This looks really nice! Thanks for the fix! Could you squash the commits please?

I notice it is a "draft" PR, could you make it into a normal PR once is ready to be reviewed?

Thanks again!

@alexdutton
Copy link
Member Author

Draft> Yep. I haven't yet got the tests working locally, so was going to do that, and then add a test with a contrived npm script faulure. If you're happy to have it without the additional test then that's fine and I'll squash and de-draft it :-)

Without this, npm errors pass silently, potentially leading to 'successful'
incomplete webpack builds.

Also adds a test that takes the simple project, removes a necessary file, and
checks whether the build is reported as failing.
@ppanero
Copy link
Member

ppanero commented May 19, 2020

@alexsdutton what you describe sounds nice! tests are <3 Thanks!

Just FYI, this is related to inveniosoftware/pynpm#6

@alexdutton alexdutton marked this pull request as ready for review May 19, 2020 11:47
@alexdutton
Copy link
Member Author

@ppanero Test added, and ready for review. :-)

@@ -87,6 +88,14 @@ def test_project_no_scripts(brokenprj):
project.buildall()


def test_project_failed_build(simpleprj):
# Remove a file necessary for the build
os.unlink(os.path.join(os.path.dirname(simpleprj), 'index.js'))
Copy link
Member

Choose a reason for hiding this comment

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

Since simpleprj does not define a scope, it is function. However, if that were to change in the future this could break other tests since it is not linked again.

I am not against leaving it as is, since it shouldn't change. However, it is breaking a fixture purposely 😅 Let's see what others think @ntarocco @fenekku @slint

Copy link
Contributor

Choose a reason for hiding this comment

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

I see your point, it might happen true. In my opinion we can leave it like this and eventually fix it in the future.

@ntarocco ntarocco merged commit b036a0f into inveniosoftware:master May 22, 2020
@ntarocco
Copy link
Contributor

@alexsdutton I have merged your PR, thanks a lot for your contribution.
I forgot to ask you who is the copyright holder of this contribution (you, your employer?) because we need to keep track of it (see here)

@alexdutton
Copy link
Member Author

Thanks for reviewing and accepting it, @ntarocco!

My employer, Cottage Labs LLP (@CottageLabs), will be the copyright holder for all my contributions to Invenio organisation repositories.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants