Write javascript tests to cover the refactored schedule editing code

[cewing]

This issue is a duplicate of scrumdo story number 32 currently in the backlog

In this comment the refactor was suggested, and tests requested. The refactor is now complete, and we need to add tests to cover the js written. This came up during renaming the package, as an error on my part in missing a name resulted in a broken UI that was not caught by test failures.

[cguardia]

@bdero now that I have been changing the schedule js, I'd like to finish the tests, but I ran into a problem running them. I sent you an email. I wonder if you have any suggestions? Thanks.

[pdpinch]

@bdero did you get a chance to follow up with Carlos about JavaScript tests?

[cguardia]

Sorry this took so long, but just pushed the schedule tests to the testjs branch.

[pdpinch]

Sorry, I can't find the branch. Can you post the URL?

If you think it's ready for us to review, I believe the best thing to do is submit a WIP PR to edX, and tag @bdero, @carsongee and @pdpinch

[cguardia]

My bad, forgot to mention that the work was done in the jazkarta repository: https://github.com/jazkarta/edx-platform/tree/testjs

It's working and covers most of the code, though there are minor things that are not tested.

[cguardia]

Is there a way to measure coverge of js tests? @bdero?

[bdero]

I haven't looked at javascript coverage reports in edx-platform, but grepping for jscover reveals that it's probably built into their tooling.

➜  edx-platform git:(bdero/sga-experimental) grep -r jscover ./                                         (bdero/sga-experimental⚡)
./.gitignore:jscover.log
./.gitignore:jscover.log.*
./scripts/run_unit_tests.sh:echo 'Configuring jscover...'
./scripts/run_unit_tests.sh:mkdir -p jscover-dist && wget http://files.edx.org/testeng/JSCover-1.0.2.zip -P jscover-dist && unzipjscover-dist/JSCover-1.0.2.zip -d jscover-dist/ && cp jscover-dist/target/dist/JSCover-all.jar jscover-dist && export JSCOVER_JAR=$PWD/jscover-dist/JSCover-all.jar
./scripts/run_unit_tests.sh:echo 'jscover configured'

Is there a jscover-dist directory or jscover.log created after you run the tests?

I didn't find any information about it skimming through the docs.

[bdero]

Any reason this is assigned to me?

[cguardia]

@bdero sorry, re-assigned to myself.

[pdpinch]

@bdero I was hoping you could help with code review.

On May 5, 2015, at 2:31 PM, Brandon DeRosier notifications@github.com wrote:

Any reason this is assigned to me?

— Reply to this email directly or view it on GitHub.

[cguardia]

@pdpinch @bdero a code review would be nice. I believe I have covered the most important functionality, but there are some things that are hard to test, like the date popups, which I skipped for the time being.

The actual date change functions that the popup calls are tested, so I think that's all right. However, if we do need 100% coverage, I would have to work on that a bit more. I was just discussing this with @cewing, and we agreed that I should at least measure the current coverage. We found a document that has some information about this, which I will follow today to get some numbers.

Meanwhile, Cris is going to talk to Sarina about how they prefer the PR to be made, since it appears WIP requests are not something they like.

[cguardia]

I added more tests. Getting jscover to run was a bit troublesome, but it works. Coverage is at 70%, but the date modal tests would get that to over 90%. However, I was not able to test that today. Lots of JS in the core is below 100%, though.

Sarina says it's best for them if we make the PR once it's ready, so it would be very helpful if you can review the code and confirm tests pass for you. I merged our master into the testjs branch, and Cris merged edx master there recently, so we are very close to what they have.

[pdpinch]

@bdero

[bdero]

I ran the tests and they pass for me. All test coverage increase is a :+1: and I think it looks good for review. Please submit a PR to edX.

[cguardia]

@pdpinch, @bdero, I created the PR: https://github.com/edx/edx-platform/pull/8019

[cguardia]

@pdpinch updated PR.

[cguardia]

@pdpinch this was finally merged tonight.

[cguardia]

@pdpinch can we close this now?

[pdpinch]

Awesome, thanks! (I missed the notification)