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

Fix uncaught exception when undoing coauthored commit #1558

Merged
merged 7 commits into from Jun 29, 2018

Conversation

Projects
None yet
3 participants
@annthurium
Copy link
Contributor

annthurium commented Jun 28, 2018

Fixes #1538

When undoing a co-authored commit, we were passing a plain object instead of an Author model. Since this object doesn't have a matches method, it blew up during rendering and shut down the whole tab.

bitmoji

Test plan:

  • manually test undoing a co authored commit
  • unit test to ensure that Author model is passed as a prop.
@@ -713,13 +713,15 @@ describe('GitTabController', function() {
const repository = await buildRepository(workdirPath);
sinon.spy(repository, 'undoLastCommit');
fs.writeFileSync(path.join(workdirPath, 'new-file.txt'), 'foo\nbar\nbaz\n');
const coAuthorName = 'Janelle Monae';

This comment has been minimized.

@annthurium

annthurium Jun 28, 2018

Author Contributor

one thing I learned is that fubar (where foo bar comes from) is a military term for "effed up beyond all recognition." Which uhhh, isn't an image I really want to call to mind, so I'm going to try to use more creative metasyntactic variables and test fixture data much as I can.

This comment has been minimized.

@smashwilson

smashwilson Jun 29, 2018

Member

Hah, sure, that's fair.

I've been tending to use aaa / bbb / ccc style placeholders, or suffixing with a one-up number (user0, user1). If I can I'll also try to name things after the role I'm intending them to play in the test (Pull Request With An Empty Description).

... But I get lazy sometimes too 😉

@annthurium

This comment has been minimized.

Copy link
Contributor Author

annthurium commented Jun 28, 2018

@smashwilson: as far as review feedback goes:

  • do you think there might be any other places hanging around where we're not using an Author model and should be?
  • are there other unit test cases I'm not thinking of?

@annthurium annthurium requested a review from smashwilson Jun 28, 2018

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jun 28, 2018

Coverage Status

Coverage increased (+0.09%) to 73.285% when pulling 64da7da on tt-18-jun-co-authors-bug into 9c588db on master.

@smashwilson
Copy link
Member

smashwilson left a comment

are there other unit test cases I'm not thinking of?

Hmm, taking a look at the coverage data, one notable thing is that it looks like we don't have any tests covering the new co-author form at all. So opening that, cancelling, and confirming a new co-author would be things it would be good to test.

(Er, I see I didn't add any tests for the excluded co-author feature either. 🙈 )

Actually, it looks like we don't have any explicit tests for the co-author feature at all. I'm guessing that's because the bulk of the heavy lifting is done by react-select anyway. Maybe it would be nice to cover the methods that we pass there as props - matchAuthors(), renderCoAuthorListItem(), and so on - so we can assert that they obey react-select's stated contracts.

do you think there might be any other places hanging around where we're not using an Author model and should be?

I sure hope not 😆

As far as I can tell with a few greps through the codebase:

  • The only places that Authors should be found are in the selectedCoAuthors prop and updateSelectedCoAuthors() function prop arguments, which are passed to CommitView from GitTabController.
  • selectedCoAuthors is only written by the GitTabController.updateSelectedCoAuthors() function body.
  • updateSelectedCoAuthors() has one call site in GitTabController (which is what you fixed!) and three in CommitView. The other three in CommitView appear to pass Author arrays correctly.

So I think that means we're fairly safe. 🤞

@@ -713,13 +713,15 @@ describe('GitTabController', function() {
const repository = await buildRepository(workdirPath);
sinon.spy(repository, 'undoLastCommit');
fs.writeFileSync(path.join(workdirPath, 'new-file.txt'), 'foo\nbar\nbaz\n');
const coAuthorName = 'Janelle Monae';

This comment has been minimized.

@smashwilson

smashwilson Jun 29, 2018

Member

Hah, sure, that's fair.

I've been tending to use aaa / bbb / ccc style placeholders, or suffixing with a one-up number (user0, user1). If I can I'll also try to name things after the role I'm intending them to play in the test (Pull Request With An Empty Description).

... But I get lazy sometimes too 😉

assert.deepEqual(wrapper.find('CommitView').prop('selectedCoAuthors'), [
{name: 'Foo Bar', email: 'foo@bar.com'},
]);
assert.deepEqual(wrapper.find('CommitView').prop('selectedCoAuthors'), [expectedCoAuthor]);

This comment has been minimized.

@smashwilson

smashwilson Jun 29, 2018

Member

It's really helpful that deepEqual works on object instances 😅

This comment has been minimized.

@annthurium

annthurium Jun 29, 2018

Author Contributor

YUP

@annthurium

This comment has been minimized.

Copy link
Contributor Author

annthurium commented Jun 29, 2018

Hmm, taking a look at the coverage data, one notable thing is that it looks like we don't have any tests covering the new co-author form at all. So opening that, cancelling, and confirming a new co-author would be things it would be good to test.

I'm confused - it looks like the form states are tested in co-author-form.test.js. Is the coverage report not picking this up? Or am I reading the report incorrectly?

@annthurium

This comment has been minimized.

Copy link
Contributor Author

annthurium commented Jun 29, 2018

I'm confused - it looks like the form states are tested in co-author-form.test.js. Is the coverage report not picking this up? Or am I reading the report incorrectly?

nevermind - I see now that the report is saying we don't have any coverage of the co-author functionality in CommitView. Added some tests.

annthurium added some commits Jun 29, 2018

@annthurium annthurium merged commit 380a503 into master Jun 29, 2018

4 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.09%) to 73.285%
Details

@annthurium annthurium deleted the tt-18-jun-co-authors-bug branch Jun 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.