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

Add tests #13

Closed
wants to merge 15 commits into from
Closed

Add tests #13

wants to merge 15 commits into from

Conversation

lb-
Copy link
Contributor

@lb- lb- commented Jun 16, 2018

This includes the changes in - #12

Adds a very basic test suite, will only work on Linux/macOS but should serve as a starting point for developers coming in.

@lb-
Copy link
Contributor Author

lb- commented Mar 10, 2019

gonna close this one as it looks like this will not be merged in - let me know if you want me to re-open.

thanks

@lb- lb- closed this Mar 10, 2019
@joshwcomeau joshwcomeau reopened this Mar 10, 2019
Copy link
Owner

@joshwcomeau joshwcomeau left a comment

Choose a reason for hiding this comment

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

Looks great! Sorry that this sat for so long :(

It looks like there are some conflicts, since the other PR I merged makes similar changes. The conflicts seem easy enough to resolve (the first two are just Git being weird about end-of-file newlines), but let me know if you want to tackle them. The only thing I'm not sure about is how to resolve the conflicts in the lockfiles; my general approach is to just delete and regenerate them, but them you lose the benefits of dependency version locking.

But yeah, I'm happy to commandeer and get this landed. Really appreciate the work, and sorry again about the humongous delay!

(As an aside, we should probably delete package-lock.json and just use Yarn, or vice-versa. Didn't mean to wind up with both)

done();
});
});
});
Copy link
Owner

@joshwcomeau joshwcomeau Mar 10, 2019

Choose a reason for hiding this comment

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

This suite feels a bit to me like it's testing echo rather than anything written in userland. Just curious, in what situation would we worry about this failing? Windows, maybe? If so, maybe we should log something to console.error to explain.

@@ -0,0 +1,52 @@
const path = require('path');
const execa = require('execa');
Copy link
Owner

Choose a reason for hiding this comment

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

Never heard of this dep, but it sounds interesting! child_process is a pain to work with.

@lb- lb- closed this by deleting the head repository Apr 15, 2023
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.

2 participants