Skip to content
This repository was archived by the owner on Mar 3, 2023. It is now read-only.

Conversation

@pbiggar
Copy link
Contributor

@pbiggar pbiggar commented Aug 1, 2016

This is my first PR against atom, so let me know if I've gotten anything wrong.

It's not currently possible to see what specs pass when you ctrl-alt-cmd-P. You see that some tests pass (via the red and green dots), but you can't see which ones (except by process of deduction by looking at which fail)

This PR adds the description of the spec, as well as all it's parent suites, to a tooltip so you can see what's passing and failing on mouseover.

I couldn't find tests for atom-reporter. Given the simplicity of the change and the overhead of learning how to write tests for this component, it didn't seem worthwhile. If there are already specs for it, please point me to them and I'll be happy to add a quick test.

Before: (no tooltip)
screenshot 2016-08-01 10 24 12

After: (tooltip, showing suite and test name)
screenshot 2016-08-01 11 23 07

@winstliu
Copy link
Contributor

winstliu commented Aug 1, 2016

I'm 👍 on this. Any chance you can upload an image of this in action?

@pbiggar
Copy link
Contributor Author

pbiggar commented Aug 1, 2016

@50Wliu Cool, thanks. Just added an image.

@pbiggar
Copy link
Contributor Author

pbiggar commented Aug 1, 2016

I looked at the tests on circleci (heh), and it looks like an intermittent failure in a different component. Tests pass on the other services.

@winstliu
Copy link
Contributor

winstliu commented Aug 1, 2016

In your picture it looks like the two describes are flipped? Shouldn't it be "paredit-el" first and then "Basic Insertion Commands tests"? Also, the actual spec is missing the leading it.

@pbiggar
Copy link
Contributor Author

pbiggar commented Aug 1, 2016

You are absolutely correct! Great catch, thanks! I hadn't thought about the "it" either - I'll fix that.

@pbiggar
Copy link
Contributor Author

pbiggar commented Aug 1, 2016

@50Wliu I forgot to reverse the order when traversing the parents. And great suggestion on adding "it" - I'm new to BDD so it hadn't occurred to me.

Code and "after" screenshot are updated.

@winstliu
Copy link
Contributor

winstliu commented Aug 1, 2016

@atom/feedback any comments?


specTitle: (spec) ->
s = spec.suite
parentDescs = while s = s.parentSuite
Copy link
Contributor

Choose a reason for hiding this comment

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

This loop's a bit confusing – mind refactoring this so all these assignments aren't inlined?

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the innermost suite (spec.suite) will not have its description added to the parentDescs array. Is that true / intentional? I'm not too familiar with how the specs and suites are represented in Jasmine, but I would think that spec.suite would correspond to the innermost describe call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are correct. I discovered the same thing in the refactor :)

@maxbrunsfeld
Copy link
Contributor

⚡ This is a great improvement; thanks so much for adding it @pbiggar! And nice to see you again, also. I left one question, but otherwise I'd love to 🚢 this.

@maxbrunsfeld
Copy link
Contributor

I couldn't find tests for atom-reporter. Given the simplicity of the change and the overhead of learning how to write tests for this component, it didn't seem worthwhile.

Yeah, there are no tests on it currently; thanks for checking.

It's not currently possible to see what specs pass. This adds the
description of the spec, as well as all it's parent suites, to a
tooltip so you can see what's passing and failing on mouseover.
@maxbrunsfeld maxbrunsfeld merged commit 368b677 into atom:master Aug 3, 2016
@maxbrunsfeld
Copy link
Contributor

This will be released in Atom v1.11.

@pbiggar
Copy link
Contributor Author

pbiggar commented Aug 3, 2016

Awesome, thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants