Obojobo
Obojobo copied to clipboard
ucfopen
Issue/1644 objectives
The first few commits seem a bit out of place, but overall this PR looks much cleaner than the last few.
In the interest of not packing too many changes into one release, and in anticipation of (hopefully) being able to release dev/28 soon, this one might have to wait until dev/29 - but provided there aren't too many overlapping changes, it shouldn't be hard to manage merging in updates from upstream.
I'll try to review this within the next week or two. In the meantime, it looks like at the very least the pre-merge tests are failing at the 'does prettier need to run' step - you should be able to solve that by running yarn pretter:run in the root directory so that prettier can make any necessary updates to the source code.
Hey, may I know how to fix this test? I tried editing snapshots but it still gives me this error while testing.
Hey Brandon, I not sure how to fix this block, could you please give me some kind of input? Thank you!
Sorry, I've been pulled in different directions recently and wasn't able to focus on this.
I can't really speculate as to the cause of the snapshot failures, I will probably have to run the code locally and follow the flow to have any idea. I'll try to make time this or next week to try figuring out what's going on.
After updating snapshots, the only remaining test failure was More Info Box moves up and down. I'm guessing this is the test you meant. It looks like no modifications were made to the tests for the MoreInfoBox component, and miraculously this was the only test that failed (not counting snapshots) due to the presence of the new Objectives area.
The failure here is that a moveNode function is expected to be called twice, but is only called once. This is due to the buttons being 'clicked' in the test. The test refers to the buttons by their indices (e.g. component.find('button').at(6)). Since the introduction of the Objectives area to the MoreInfoBox component, these indices no longer refer to the same buttons. Before, the 'Move Up' button was at index 6 and the 'Move Down' button was at index 7. That is no longer the case; since the Objectives area adds another button, the 'Move Up' button is now at index 7 and the 'Move Down' button is at index 8. Updating these indices in the test resolves the failure.
After fixing that, the tests all pass but coverage is no longer at 100%, so you'll have to address that.
That's correct.
For posterity, you can generate coverage reports (using lcov, which I prefer) via yarn test --coverage --coverageReporters=lcov. This will create a /coverage/lcov-report directory, the easiest way to view the coverage report is to open the index.html file located in that directory in your browser. The report will indicate uncovered statements, branches, lines, etc. per file. You can use that to determine where additional tests are required to address the missing coverage.
I will sometimes also run the above command with the --watchAll flag so that coverage is re-run when I make a change to the tests, but I would also advise that you use the available filters while the tests are running to focus on a specific file or folder at a time.
So I have to use the coverage reports to write more test cases, and only then it will be pushed into production?
The lcov reports are helpful in determining what you haven't tested yet, but in general I would say that you're not writing tests to cover specific lines of code, you're writing them to cover specific scenarios to make sure everything is working the way it's expected to work.
Once the new code is fully covered and the new features have been reviewed, assuming no further changes are necessary, the pull request can be merged into whatever the current dev branch is.
Releases are less regular than they were in the past, but once something has made it into the dev branch it will (eventually) be run on QA for a while to confirm everything runs properly, and will then be solidified in a release and pushed to production some time after that.
@zachberry and possibly @iturgeon may want to look this over since they were running the show at this feature's inception. I can see that it appears to be working properly but I don't think I have the necessary context to say that it's satisfying all of the requirements.
Assuming I figure out why the tests pass locally to allow the commit but fail here.