Freecodecamp: modify pull request template for testing guidance

[ivantedja]

I think it is better to change the instructions for Pre-Submission Checklist ?

All new and existing tests pass the command npm run test-challenges. Use git commit --amend to amend any fixes.

changed with

All new and existing tests pass the command npm test. Use git commit --amend to amend any fixes.

My previous pull request didn't pass the travis check because of linting problem although I did follow instructions from Pre-Submission Checklist to run npm run test-challenges.

[Bouncey]

LGTM

First-timers only

Edit '.github/PULL_REQUEST_TEMPLATE.md' in the manner presented by @ivantedja

Be sure to use CONTRIBUTING.md to get set up to make the edit. If you get stuck, ask for help in the contributors chat on gitter.

Good luck, happy coding and thanks for contributing 🎉

[dhcodes]

So npm test will run the same tests as travis-CI?

[Bouncey]

Yes @dhcodes

Every day is a school day

[ivantedja]

@Bouncey CONTRIBUTING.md looks good to me, I meant .github/PULL_REQUEST_TEMPLATE.md

[dhcodes]

fixed it for @Bouncey

[ghost]

No, I believe npm test runs a different set of tests that npm run test-challenges. I believe npm test doesn't work on Windows.
cc/ @FreeCodeCamp/issue-moderators

[BKinahan]

Right, npm test also runs the file linting. The text in the PR template is correct that npm run test-challenges should be used to make sure all current and new tests pass, but should perhaps be amended to use npm test (with a text change reflecting what that command does differently from npm run test-challenges) so that linting errors can be caught before the PR stage.

[dhcodes]

But does it work on windows...

And will it blend?

[ivantedja]

@dhcodes I don't have Windows, but it seems that it won't work in Windows. I saw this in CONTRIBUTING.md:

When you are ready to share your code, run the test suite npm test and ensure all tests pass. For Windows contributors, skip the jsonlint pretest run by using npm run test-challenges, as jsonlint will always fail on Windows, given the wildcard parameters.

and this

You should have ESLint running in your editor, and it will highlight anything doesn't conform to Free Code Camp's JavaScript Style Guide (you can find a summary of those rules here. Please do not ignore any linting errors, as they are meant to help you and to ensure a clean and simple code base. Make sure none of your JavaScript is longer than 80 characters per line. The reason we enforce this is because one of our dependent NPM modules, jsonlint, does not fully support wildcard paths in Windows.

We might need to change wording targeting Windows (run npm run test-challenges only) and non-Windows users (run npm test). But I'm not sure will it cause confusion or not 😅

[dhcodes]

Will #10888 remove the necessity for this or am I reading that wrong? Seems like @BerkeleyTrue's PR may be a better option since it would work on both Mac and Windows.

[ivantedja]

@dhcodes I'm sure that PR will have the same problem

[Bouncey]

Is there another option for JSON linting that does support wildcard paths in windows? That would solve everything

[Bouncey]

Or, could this line be the reason wildcard paths just aren't working properly?

Should it be seed/challenges/**/*.json to iterate through all folders?

[ivantedja]

I think it is this line.

npm test will run pretest, test, posttest under scripts: {}

I'm not really sure which one cause the fail though as I don't have Windows to test it 😢

[BerkeleyTrue]

@Bouncey I think you are correct. The library that does the json linting itself does not take precautions for windows paths.

[BerkeleyTrue]

Can someone verify where it fails on windows?

[ivantedja]

@dhcodes post a gist in here. He even installed Windows to test it, thanks for you effort! 🎉

[dhcodes]

Didn't have to install windows, only set up a dev environment 😉 . It's a whole new world.

[systimotic]

Windows user here! I did some investigation. There are two parts that don't work on Windows.

jsonlint

jsonlint doesn't work because of an issue with wildcard paths, as mentioned before. I took a quick look at its source. The CLI just takes the parameter passed in the command line and reads it with fs.readFileSync(). It's not iterating over multiple files anywhere, or doing anything with a glob. I suspect readFileSync might concatenate all the files on Unix systems if the argument is a glob, but I haven't been able to test this or find any evidence. On Windows, it's looking for a file named *.json, which isn't there.

Possible solutions

gulp-jsonlint

Gulp does work! The plugin is just a tiny wrapper around the package, but Gulp's stream does look at the glob and get all the files, and plugin loops over them and runs them all through jsonlint. Currently, it doesn't exit if there's an error. The plugin does have that functionality built in, so it would only be a small change. Cons of this is that it clutters the gulpfile. We would have to add some new tasks. It might also add a lot of bootstrap. The test runs in 500-2000ms, but if I haven't run it for a while, gulp usually takes a while to start up (my estimate is that it takes from about 30 seconds to 3 minutes). This might only be a Windows thing though. Seeing as though you have the startup time anyway if you're going to develop, I don't think this is a major issue. It would only be an issue if this also is the case on the CI servers (they usually have much less CPU power than my laptop). I get the feeling the startup time is also a Windows thing. Please let me know if you also have the startup time on Mac or Linux.

Custom script

From my research, I believe it should be fairly easy to write a custom linting script. It would add a dependency, glob, but not any size, as glob is already downloaded. Gulp depends on it, and probably a few other packages as well. This wouldn't have the startup time problems that Gulp has, but it would be one more thing to maintain. I believe it wouldn't need a lot of work, but I'm not going to make that call. I _am_ going to write this script and give a proper comparison. I will post a new comment once that's done.

test-js / tape

All of the commands run by the test-js script do not work. For test-js-[subject], the command is "tape -r babel-register '[subject]/**/*.test.js' | tap-spec". After a lot of experimenting, I found that this was another issue with quotes, similar to #11172. I will be submitting a PR to fix this.

---

I'd love to hear what you think we should do with jsonlint.

[BerkeleyTrue]

@systimotic Does replacing jsonlint with jsonlint-cli fix the issue?

[systimotic]

It looks promising, I'll check it out right now. If it works, should I submit a PR?

[BerkeleyTrue]

A PR would be wonderful!

This should be a separate PR from the test-js issue

[systimotic]

Both PR's were merged, the test suite works on Windows again!
@Bouncey I think this can be assigned to first-timers-only again

[charlottetan]

Hey guys, I'd like to take a shot at this, is the change just updating the wording?

[QuincyLarson]

@charlottetan that's correct - I believe it's just the text changes proposed by @ivantedja. You have dibs on this - let us know if you have any questions 😄

[ivantedja]

That's correct, it is only wording changes.