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

Add a spec for ResultViewComponent #1440

Merged
merged 2 commits into from
Oct 13, 2018

Conversation

hpierce1102
Copy link
Contributor

#813

This adds a basic test for ResultViewComponent. I was hoping to get farther and write more useful tests, but ran into issues with mocking some internals of the class.

For instance, I wanted to demonstrate the clicking on the topmost <div> actually ran openInEditor, but struggled to mock openInEditor. I also wanted to demonstrate that clicking icon-clippy actually copied stuff to the clipboard, but figuring out how to make that actually button appear for a test was outside of my React skills (I got to a point where it looked like it had something to do withref = { ref => {]}) 😕.

I'd be more than happy if someone had some advice to offer in terms of adding more value to these tests. Or if someone thinks this is useful enough to merge without more tests - I suppose that'd be fine too.

Copy link
Member

@lgeiger lgeiger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for contributing! We really appreciate your help to improve test coverage 🎉

I added a few suggestions, that would simplify the code.

I also wanted to demonstrate that clicking icon-clippy actually copied stuff to the clipboard, but figuring out how to make that actually button appear for a test was outside of my React skills.

Just guessing from the the code, I would say the button is not rendered because there's no text to be copied in the output. I'd be happy to take a closer look later this week.


let destroyInvoked = false;
const destroy = () => {
destroyInvoked = true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be simplified using jasmine spies:

const destroy = jasmine.createSpy("destroy");
const component = shallow(
  <ResultViewComponent
    store={mockStore}
    kernel={{}}
    destroy={destroy}
    showResult={true}
  />
);

component.find(".icon-x").simulate("click");
expect(destroy).toHaveBeenCalled();

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hpierce1102 did you want to take a crack at using Jasmine spies here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the radio silence. Yes, I'd like to look at it again and dive in Jasmine spies.

@lgeiger
Copy link
Member

lgeiger commented Oct 8, 2018

Side note: It would be great to be able to use jest snapshots for this kind of component testing. It would make things a lot easier for sure.

@BenRussert
Copy link
Member

Just guessing from the the code, I would say the button is not rendered because there's no text to be copied in the output.

This is correct. Here is the relevant line. Basically this.getAllText().length > 0 checks to see if there is any text to copy, there won't be any text to copy if the output is an image, for example.

@BenRussert BenRussert merged commit d0331b4 into nteract:master Oct 13, 2018
@BenRussert
Copy link
Member

Thanks for putting this together! I know you had some more goals you mentioned above, but I think it makes sense to merge now and please feel free to send over more PRs if you keep working on that (or anything else 😀)

Welcome to the nteract party!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants