Update contributing.md #4368

addisonElliott · 2017-11-21T14:17:49Z

Detail how to clone the source code and begin testing new features or bug fixes
Include commands for Windows and Unix for running test coverage and running tests.

Clean up some of the language in the document. Add command to run for testing on Windows

Update coverage directory

Add details about how to run test coverage

codecov · 2017-11-21T14:29:10Z

Codecov Report

Merging #4368 into master will decrease coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #4368      +/-   ##
==========================================
- Coverage   92.74%   92.71%   -0.04%     
==========================================
  Files         119      119              
  Lines        8438     8438              
==========================================
- Hits         7826     7823       -3     
- Misses        612      615       +3

Impacted Files	Coverage Δ
src/RestWrite.js	`93.14% <0%> (-0.55%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 37ceae0...ea34536. Read the comment docs.

montymxb · 2017-11-21T21:23:14Z

CONTRIBUTING.md

-* Never publish the lib folder.
+* Begin by reading the [Development Guide](http://docs.parseplatform.org/parse-server/guide/#development-guide) to learn how to get started running the parse-server.
+* Take testing seriously! Aim to increase the test coverage with every pull request. To obtain the test coverage of the project, run:
+    * **Windows**: `npm run coverage:win`


I'm wondering if we should consolidate these so it's just npm run coverage rather than one or the other for platform issues. We could keep npm run coverage:win and just make it an alias for the former.

problem is the env variables that are not processed properly on windows :/

We could modify npm run coverage to os detect and run from there? Would be a bit more code underlying, but I don't believe we'd ever need to worry about which command to tell someone to use in the future and we probably wouldn't ever change it.

I would agree that consolidating into one would be great. It's unfortunate that npm doesn't support having scripts for specific OSes but oh well.

I'll give a shot at consolidating this into one script.

(addressing both :win scripts)
test:win wouldn't be required if all test did was test - this is what test:win does - suggest

Remove test and coverage

Rename test:win to test - this will only perform testing

Rename coverage:win to coverage and ensure works on Unix/Mac by using the correct path for jasmine that works for both platforms

Can probably shorten to
cross-env MONGODB_VERSION=${MONGODB_VERSION:=3.2.6} MONGODB_STORAGE_ENGINE=mmapv1 TESTING=1 node nyc ./node_modules/jasmine/bin/jasmine.js

Actually here's what we use in another project for coverage (on Windows):
cross-env NODE_ENV=test TESTING=1 nyc jasmine

montymxb · 2017-11-21T21:23:51Z

CONTRIBUTING.md

+    * **Windows**: `npm run coverage:win`
+    * **Unix**: `npm run coverage`
+* Run the tests for the file you are working on with the following command:
+    * **Windows**: `npm run test:win spec/MyFile.spec.js`


Same thing here, there are very few underlying differences.

montymxb · 2017-11-21T21:25:30Z

CONTRIBUTING.md

+    * **Unix**: `npm test spec/MyFile.spec.js`
+* Run the tests for the whole project to make sure the code passes all tests. This can be done by running the test command for a single file but removing the test file argument. The results can be seen at *<PROJECT_ROOT>/coverage/lcov-report/index.html*.
+* Lint your code by running `npm run lint` to make sure the code is not going to be rejected by the CI.
+* **Do not** publish the *lib* folder.


@flovilmart what do you think of the language changes to the contributing here?

flovilmart · 2017-11-22T15:26:01Z

the COVERAGE_OPTION is set on the CI, so we can run the default command, and locally, you're not getting a performance hit from running npm test which would include coverage otherwise. Problem is on windows setups, those env vars are not properly interpreted. Can we just get the windows developers to use VSCode + windows linux subsystem?

flovilmart · 2017-11-22T15:27:32Z

we could even put it in a common .vscode so all contributors share the same defaults, (eslint-warning etc...) https://code.visualstudio.com/docs/editor/integrated-terminal#_configuration

addisonElliott · 2017-11-22T15:39:42Z

What about using this package: https://www.npmjs.com/package/cross-var

Similar to cross-env except it allows you to retrieve variables from Windows/Linux.

flovilmart · 2017-11-22T15:41:45Z

That seems like a great solution, all from cross-env and good with windows. Can you check it out?

flovilmart · 2017-11-22T15:42:57Z

It still would be nice to have a recommended setup for development, I know a few of the maintainers use VSCode, we also have Jetbrain licences for OSS projects, but I prefer VSCode in the end.

montymxb · 2017-11-23T03:44:43Z

Recommended setup sounds like a great README addition.

addisonElliott · 2017-11-23T16:53:21Z

Alright, so here's an update on what the exact issue with the test script and why it will not work for Windows.

First, cross-env actually supports replacing environmental variables on Windows. e.g. $ENV_VARIABLE will be replaced with the correct env. variable on Windows. But the issue lies elsewhere with running the test script on Windows.

Test & Coverage Scripts

"test": "cross-env MONGODB_VERSION=${MONGODB_VERSION:=3.2.6} MONGODB_STORAGE_ENGINE=mmapv1 TESTING=1 $COVERAGE_OPTION jasmine",
"coverage": "cross-env COVERAGE_OPTION='./node_modules/.bin/nyc' npm test",

Error on Windows

On Windows, the error message when running npm test is:

'"."' is not recognized as an internal or external command, operable program or batch file.

Why the error?

I'm going to omit the MONGODB_VERSION and other env variables besides COVERAGE_OPTION below to simplify this explanation.

Here is the test script: cross-env $COVERAGE_OPTION jasmine For npm test, COVERAGE_OPTION is not set.

On Unix, this will be replaced with an empty string by the shell before it is passed to cross-env. The actual command ran is cross-env jasmine (double space there). The shell will correctly handle this and make sure jasmine is the only argument to cross-env.

On Windows, this is not recognized so the text cross-env $COVERAGE_OPTION jasmine will be ran. cross-env sees $COVERAGE_OPTION is the command to be run and jasmine is the argument passed on to the command. Next, $COVERAGE_OPTION is replaced with an empty string and cross-env attempts to run that empty string. Hence the error.

Solutions

There are some different solutions to this problem.

Leave the test:win and coverage:win scripts.
- I've documented them and that would work fine. I definitely think we could try to consolidate the test/coverage scripts however.
Create a Javascript script that checks the OS and then runs commands for test/coverage based on OS
Try to submit an issue to cross-env and see if they think it would be worthwhile to fix.
- Could take a little bit to get the pull request accepted and a new release created
Make it so test script only tests and coverage only gets coverage.
- Solves all issues easily. Would clean up the code a bit.
- Only issue is CI sets COVERAGE_OPTION in test script. Can this be changed though?

@flovilmart, @montymxb What are your thoughts? My preference is option 4 and finding a way to change the CI to not set the COVERAGE_OPTION. Can we set it up to just run npm run coverage instead?

flovilmart · 2017-11-23T17:00:01Z

You need to run tests to have the coverage, so gathering coverage is a corner case of the running the tests. Yes we can technically change everything to accommodate windows machines. But i’d Rather have something that keeps being simple.

steven-supersolid · 2017-11-23T17:13:38Z

Is it not simple to change CI to run coverage instead of test as suggested and remove COVERAGE_OPTION? This would make package.json platform agnostic which I think can only be a good thing for open source.

montymxb · 2017-11-24T20:39:41Z

npm run coverage doesn't factor in the fact we change MONGODB_VERSION across runs. We would get coverage but it would not be necessarily true to what has been covered.

I really do think we should strive for a platform agnostic solution though, to agree with @steven-supersolid . It is a community after all 👍 .

@addisonElliott option 2 and 4 both seem tangible. The first would require us to write a JS script for our tests, but it would still need args passed. As for the second option, and this is just a thought, could we simply declare a variable containing both params in the same line and run that instead?
Just a two step, roughly like this:

CROSS_ENV_ARGS=`$COVERAGE_OPTION jasmine`
cross-env $CROSS_ENV_ARGS

If it substitutes after each line then CROSS_ENV_ARGS could work. Something I might take a look at myself just to see if it's even possible with cross-env...

flovilmart · 2017-11-24T21:09:35Z

Didn’t you mention cross-var as a proper alternative to cross-env?

addisonElliott · 2017-11-24T21:32:54Z

I looked into cross-var as an alternative. It didn't not work either because it does something very similar to cross-env.

The issue of it trying to execute a blank command still stands.

flovilmart · 2017-11-24T21:34:49Z

alright i’ll Take care of the CI, and open a new PR

addisonElliott · 2017-11-24T21:44:11Z

@flovilmart Sounds good. Would you like me to create a PR to update the scripts to a single test and coverage?

flovilmart · 2017-11-24T22:13:16Z

@addisonElliott I'm on it as well as integrating appveyor which proves to be a B****CH. #4377

flovilmart · 2017-11-24T22:43:25Z

@addisonElliott did you notice any issue with mongoldb-runner on your machine? Seems there's a pending issue on mongodb-runner, and it's preventing to run correctly on windows. mongodb-js/runner#81

addisonElliott · 2017-11-24T23:00:33Z

Yes, I've had some weird issues with mongodb runner. I'll look at that issue and see if it applies.

I had to manually start it each time I ran the tests

flovilmart · 2017-11-24T23:01:44Z

ok I'm checking what's going on appveyor, our test suite is supposedly starting / stopping a mongodb.

See those weird issue: https://ci.appveyor.com/project/flovilmart/parse-server/build/1.0.16

flovilmart · 2017-11-25T20:20:09Z

I'm good with those changes as they are!

* Update CONTRIBUTING.md Clean up some of the language in the document. Add command to run for testing on Windows * Update CONTRIBUTING.md Update coverage directory * Update CONTRIBUTING.md Add details about how to run test coverage

addisonElliott added 3 commits November 20, 2017 15:02

Update CONTRIBUTING.md

45b105e

Clean up some of the language in the document. Add command to run for testing on Windows

Update CONTRIBUTING.md

24a92eb

Update coverage directory

Update CONTRIBUTING.md

ea34536

Add details about how to run test coverage

montymxb reviewed Nov 21, 2017

View reviewed changes

montymxb added the enhancement label Nov 21, 2017

flovilmart merged commit 2b9397a into parse-community:master Nov 25, 2017

addisonElliott deleted the patch-1 branch November 26, 2017 03:57

snyk-bot mentioned this pull request Aug 8, 2020

[Snyk] Upgrade apollo-server-express from 2.15.1 to 2.16.0 #6851

Merged

mtrezza removed the 🧬 enhancement label Jul 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update contributing.md #4368

Update contributing.md #4368

addisonElliott commented Nov 21, 2017

codecov bot commented Nov 21, 2017 •

edited

Loading

montymxb Nov 21, 2017

flovilmart Nov 21, 2017

montymxb Nov 21, 2017

addisonElliott Nov 21, 2017

steven-supersolid Nov 22, 2017

steven-supersolid Nov 22, 2017

montymxb Nov 21, 2017

montymxb Nov 21, 2017

flovilmart commented Nov 22, 2017

flovilmart commented Nov 22, 2017

addisonElliott commented Nov 22, 2017

flovilmart commented Nov 22, 2017

flovilmart commented Nov 22, 2017

montymxb commented Nov 23, 2017

addisonElliott commented Nov 23, 2017

flovilmart commented Nov 23, 2017

steven-supersolid commented Nov 23, 2017

montymxb commented Nov 24, 2017

flovilmart commented Nov 24, 2017

addisonElliott commented Nov 24, 2017

flovilmart commented Nov 24, 2017

addisonElliott commented Nov 24, 2017

flovilmart commented Nov 24, 2017

flovilmart commented Nov 24, 2017 •

edited

Loading

addisonElliott commented Nov 24, 2017

flovilmart commented Nov 24, 2017 •

edited

Loading

flovilmart commented Nov 25, 2017

Update contributing.md #4368

Update contributing.md #4368

Conversation

addisonElliott commented Nov 21, 2017

codecov bot commented Nov 21, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flovilmart commented Nov 22, 2017

flovilmart commented Nov 22, 2017

addisonElliott commented Nov 22, 2017

flovilmart commented Nov 22, 2017

flovilmart commented Nov 22, 2017

montymxb commented Nov 23, 2017

addisonElliott commented Nov 23, 2017

Test & Coverage Scripts

Error on Windows

Why the error?

Solutions

flovilmart commented Nov 23, 2017

steven-supersolid commented Nov 23, 2017

montymxb commented Nov 24, 2017

flovilmart commented Nov 24, 2017

addisonElliott commented Nov 24, 2017

flovilmart commented Nov 24, 2017

addisonElliott commented Nov 24, 2017

flovilmart commented Nov 24, 2017

flovilmart commented Nov 24, 2017 • edited Loading

addisonElliott commented Nov 24, 2017

flovilmart commented Nov 24, 2017 • edited Loading

flovilmart commented Nov 25, 2017

codecov bot commented Nov 21, 2017 •

edited

Loading

flovilmart commented Nov 24, 2017 •

edited

Loading

flovilmart commented Nov 24, 2017 •

edited

Loading