Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Hide steps for successful scenarios #97

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
2 participants
Contributor

magnars commented Mar 1, 2013

This will hide the steps of successful scenarios, so that I can see the output of the one failing test, and not have to scroll past a hundred green lines to find it.

I guess this could have been implemented better, but that would mean rewiring the entirety of ecukes-run and ecukes-stats to not rely on side-effects, and instead gather the data and then act on it. This was a way shorter route to the goal. Hope that is satisfactory.

It can be toggled with the var ecukes-hide-steps-for-successful-scenarios, but I have not created any command line flags for this.

For some reason that baffles me, I have not been able to get one of the updated tests to actually pass. Instead I get this doozy:

Test run-scenario condition:
(ert-test-failed
 ((should
   (equal expected messages))
  :form
  (equal
   ("  Scenario: Simple")
   ("  Scenario: Simple"))
  :value nil :explanation
  (list-elt 0
            (array-elt 0
                       (different-atoms ... ...)))))

To my eyes (and my copy-paste) these are identical.

I also removed the duplicate implementation of ecukes-print-message in both ecukes-print.el and test-helper.el.

@rejeep rejeep commented on the diff Mar 1, 2013

ecukes-print.el
@@ -80,29 +81,31 @@
"Print INTRO."
(let ((ecukes-print-offset 0)
(header (ecukes-intro-header intro)))
- (ecukes-print-message "Feature: %s" header)
+ (ecukes-print-message "Feature: %s" (s-trim header))
@rejeep

rejeep Mar 1, 2013

Contributor

Do we need to (require 's) as well? I get no warning though...

@magnars

magnars Mar 1, 2013

Contributor

Hah, that would be neater at least. :-) I guess s is required already, so it is only for macros that the require is needed right in the file.

@rejeep

rejeep Mar 1, 2013

Contributor

Better safe than sorry :)

@rejeep rejeep commented on the diff Mar 1, 2013

test/ecukes-print-test.el
@@ -141,7 +141,7 @@
"simple"
(lambda (feature intro scenarios background steps)
(ecukes-print-intro intro)
- (let ((expected (list "Feature: Simple" " This" " Is" " Simple" " ")))
+ (let ((expected (list "Feature: Simple" " This" " Is" " Simple")))
@rejeep

rejeep Mar 1, 2013

Contributor

Why not the space between the feature header and the scenarios?

@magnars

magnars Mar 1, 2013

Contributor

Two reasons:

  • I've got LOTS of scenarios, so even with the collapsed steps, there's not room for all that whitespace and still always see the errors right there in the window.
  • The grouping is clearer without the space.

Instead of:

Feature: My feature 1

  Scenario: 1
  Scenario: 2
  Scenario: 3

Feature: My feature 2

  Scenario: 1
  Scenario: 2
  Scenario: 3

Feature: My feature 3

  Scenario: 1
  Scenario: 2
  Scenario: 3

This looks better (in my opinion) and saves space (not an opinion):

Feature: My feature 1
  Scenario: 1
  Scenario: 2
  Scenario: 3

Feature: My feature 2
  Scenario: 1
  Scenario: 2
  Scenario: 3

Feature: My feature 3
  Scenario: 1
  Scenario: 2
  Scenario: 3
@rejeep

rejeep Mar 1, 2013

Contributor

Agreed! :)

Contributor

rejeep commented Mar 1, 2013

Hey,

Really liking this! A few comments:

It can be toggled with the var ecukes-hide-steps-for-successful-scenarios, but I have not created any command line flags for this.

I think a flag should be added for this. I think the collapsed view is a great default. But it should be simple to switch to the expanded view.

For some reason that baffles me, I have not been able to get one of the updated tests to actually pass. Instead I get this doozy:

Make sure you don't have it wrapped in any ansi colors. Been there, done that, a million times...

I see that a failing scenario header will be white. Is this intentional? I don't mind the color at all, it's easier to see, but I would like to see a test for ecukes-print-scenario-header were the second argument is successful.

Contributor

magnars commented Mar 1, 2013

I think a flag should be added for this. I think the collapsed view is a great default. But it should be simple to switch to the expanded view.

Yes, I agree. However, writing those shell scripts is really not my forte. I looked at it, blinked a couple of times, and then hoped maybe you would do it. :-)

Make sure you don't have it wrapped in any ansi colors. Been there, done that, a million times...

I can have hidden ansi colors in the strings in my emacs? Or, how does that work? Or did I not understand?

I see that a failing scenario header will be white. Is this intentional?

Yes, just to make it easier to spot visually. It is only green when collapsed (to show it as successful).

I don't mind the color at all, it's easier to see, but I would like to see a test for ecukes-print-scenario-header were the second argument is successful.

Agreed. I was so annoyed at that test I couldn't get to pass, that I just got the other tests running and pushed to ask you, instead of adding more tests.

Contributor

magnars commented Mar 1, 2013

Make sure you don't have it wrapped in any ansi colors. Been there, done that, a million times...

Hah, I figured it out. The spaces were in green too, and shouldn't have been. Yay!

Contributor

magnars commented Mar 1, 2013

I have fixed all that we've talked about (except for the command line flag) and squashed it together into one nice commit.

Contributor

magnars commented Mar 1, 2013

This has me pretty excited :-)

Screen shot 2013-03-01 at 22 10 46

Contributor

rejeep commented Mar 1, 2013

I agree, looks awesome. Can you do that in Cucumber?

Take a look at 7f92412 Watcha think?

Contributor

magnars commented Mar 1, 2013

Oh hey, it's in elisp. Looking good. :)

It's been years since I used Cucumber, so I don't know. I hope so for their sake.

Contributor

rejeep commented Mar 1, 2013

I just realized that backgrounds are still printed. Probably should be the same fail => expand logic there...?

Contributor

magnars commented Mar 1, 2013

Oh. Are they repeated for each scenario? In that case, certainly. If they're once per feature, it might require some thinking. Only hide if all scenarios pass, I guess?

Contributor

rejeep commented Mar 1, 2013

They run for each scenario, but are only printed once. This is from drag-stuff:

drag-stuff-background

Only hide if all scenarios pass, I guess?

Sounds like a good idea. I guess the Background: header should be printed to indicate that there's a background even if all pass.

Contributor

rejeep commented Mar 1, 2013

I also realized that a test for ecukes-run-scenario should be added. There's only one now (https://github.com/magnars/ecukes/blob/8f7f565e2cda865526a5544c55f7d9e3e6c7a45a/test/ecukes-run-test.el#L193-L210). You changed that one and removed the steps printing, but I think we should have two tests. One that only prints the header and one that prints the header and all steps. Don't you agree?

Contributor

magnars commented Mar 2, 2013

I added the missing scenario test.

I also made the background steps collapse, but they do so if all background steps are successful. That's about the best I could do, given the limitations of the side-effect based printing in ecukes.

To be able to do what we were talking about, we'd either have to rewire the entire thing to build up a large data structure before printing anything. Or the output-buffering could be enriched with a system for layered buffering. The first is a too large undertaking for me this morning, and the last I found to be a too complex solution for little gain.

Contributor

rejeep commented Mar 2, 2013

Running and printing is about the most complex part of Ecukes. An awesome solution would be to create some sort of reporter interface and create multiple reporters, isolated from each other. It becomes very complex when there are conditions all over the place. So a few reporters would be, expanded, collapsed, dotted, rspec, json, markdown, etc...

Btw, if a step in the background fails, the background does not expand.

Contributor

magnars commented Mar 2, 2013

Yes, to reach that goal, running the tests have to stop printing stuff out, and instead result in a data structure of every interesting fact about the run. Then different reporters could use that.

However, that is an entirely different pull request. :)

I'll take a look at why the background does not behave as expected.

Contributor

magnars commented Mar 2, 2013

It works on my machine (tm). And the tests seem to verify it. Care to check again?

Contributor

rejeep commented Mar 2, 2013

However, that is an entirely different pull request. :)

Indeed. But this one inspired me! :) See rejeep#100

It works on my machine (tm). And the tests seem to verify it. Care to check again?

Allright, will check again!

Contributor

magnars commented Mar 2, 2013

A fitting issue number, I must say. :)

Contributor

rejeep commented Mar 2, 2013

Indeed! :)

Contributor

rejeep commented Mar 2, 2013

Now I got a different result:

background

If we are really picky I guess that if a background fails, there's no point in expanding any scenario, but all should be collapsed. Maybe we could wait with this for the big 100 if it's hard to implement.

Contributor

magnars commented Mar 2, 2013

It would have to be slightly hacky. Like checking background-success in ecukes-run-feature, and then not run the scenarios at all, just do something along the lines of:

(--each scenarios
  (ecukes-print-message (ecukes-print-scenario-header it 'skipped)))

And make the print-header function take a status symbol instead of a success boolean.

But then the # of skipped steps is wrong. Doing it right is something for #100 I think.

Thankfully breaking Backgrounds are much rarer.

Contributor

rejeep commented Mar 3, 2013

Agreed, can you (if you haven't already) verify this branch on your Ecukes project and I will do the same, just to make sure we don't break anything. Then I'll merge it!

Contributor

magnars commented Mar 3, 2013

How do you mean verify? I've been using it since making it, with much joy. :) The tests run also. Anything else I should do?

Contributor

rejeep commented Mar 3, 2013

For each release of Ecukes I try it out on a few projects to make sure I don't break anything. It's quite well unit tested, but still, they don't catch em all, so I just want to make sure.

Contributor

rejeep commented Mar 5, 2013

Just realized I never merged this, will do soon!

Contributor

rejeep commented Mar 6, 2013

I actually encountered a few issues when I was trying this out on drag-stuff.

  1. It seems to mess up py-strings:

master
drag-lines-py-string-master

hide-steps-for-successful-scenarios
drag-lines-py-string-expanded

  1. One more spacing is added in expanded view:

drag-lines-spacing

  1. There's no background spacing in expanded view:

drag-lines-background-spacing

Considering the errors, I figure you mostly run in collapsed mode! :)

Contributor

magnars commented Mar 6, 2013

That's right, I haven't looked back. :) No need to know the steps of things that work.

The py-strings issue is annoying. I wonder what that can be.

I've got my hands full these days, so I might not be able to look at it until after the week-end. If you want to give it a go, that's totally fine by me.

Contributor

rejeep commented Mar 6, 2013

I'm quite busy myself, but if I find some spare time I will.

Contributor

rejeep commented Aug 11, 2013

I'm closing this since I plan to implement reporters

@rejeep rejeep closed this Aug 11, 2013

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