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

update gherkin etc, validate against cck #1318

Merged
merged 134 commits into from
Aug 24, 2020
Merged

update gherkin etc, validate against cck #1318

merged 134 commits into from
Aug 24, 2020

Conversation

davidjgoss
Copy link
Contributor

@davidjgoss davidjgoss commented May 9, 2020

More or less copied the approach from cucumber-jvm.

To run:

$ yarn
$ yarn cck-test

(Or, run compatibility/cck_spec.ts from your IDE.)

Done:

  • Expose World and IWorldOptions in public API
  • Add a log function to the World which attaches text with the cucumber log media type
  • Emit attachments in new structure
  • Add some methods to DataTable
  • Emit Meta message at start of run - fixes Emit a Meta message at the beginning of the run. #1310
  • Keep a stopwatch of the test run, use it for a timestamp on various messages
  • Emit messages for hooks and step definitions
  • Emit messages for parameter type definitions
  • stepMatchArgumentsLists are now being emitted within testCase.testSteps
  • pickleAccepted and pickleRejected messages are now not emitted
  • testCaseFinished.testResult is now not emitted (see Move or remove will_be_retried from TestStepResult common#902)
  • testRunFinished.success is now not emitted

Questions:

  • testCase messages per CCK fixtures should be emitted together at start of run, rather than before each test case is started (non-trivial refactor - do we need this right away?)

@davidjgoss davidjgoss mentioned this pull request May 9, 2020
5 tasks
@davidjgoss davidjgoss added this to the 7.0.0 milestone May 9, 2020
@davidjgoss davidjgoss changed the title wip: validate against cck wip: update gherkin etc, validate against cck May 17, 2020
@aslakhellesoy
Copy link
Contributor

aslakhellesoy commented Aug 17, 2020 via email

@davidjgoss
Copy link
Contributor Author

And we are green.

@davidjgoss
Copy link
Contributor Author

@charlierudolph good to merge?

@charlierudolph
Copy link
Member

Awesome having cck and formatter tests passing! Let me take a look.

'nanos',
'seconds',
// errors
'message',
Copy link
Member

Choose a reason for hiding this comment

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

Are we expecting all of these to be excluded long term? I would hope with predictable ids that big block could be fixed. Do you think its worth having an issue to try to drive this list down.

Copy link
Contributor Author

@davidjgoss davidjgoss Aug 19, 2020

Choose a reason for hiding this comment

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

I agree - when I was working against a slightly earlier CDK version I think it was only the testCase ordering that made the ids not line up.

But cck has changed from using predictable ids to uuids so not sure what the intent is there or where that leaves us.

Copy link
Contributor

Choose a reason for hiding this comment

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

The predictable ids and timestamps are a dead end. The cck no longer uses them.

It's a dead end because various Cucumber implementations emit messages in slightly different order than what fake-cucumber (the engine behind the cck) emits. It would be too much work to make them all emit in the same order.

So what we opted for instead is a validation mechanism where certain values (ids, timestamps) are not compared. We pair this with smoke tests that send the generated messages through 2 of the most advanced message consumers we have - cucumber-html and json-formatter. If these don't crash and a cursory eye-balling looks OK, we're confident enough that the emitted messages are ok.

package.json Outdated Show resolved Hide resolved
@charlierudolph
Copy link
Member

charlierudolph commented Aug 19, 2020

Time reporting looks a bit off:

⋊> ~/p/cucumber-js on cck ◦ time node ./bin/cucumber-js
...
239 scenarios (239 passed)
1201 steps (1201 passed)
0m00.449s
       38.04 real        23.48 user         3.40 sys

Update: think this is due to the result of a testCase being the worst case step result and using that duration instead of the sum of the steps. I'll work on a fix for this.

Update 2 pushed fix:

⋊> ~/p/cucumber-js on cck ◦ time node ./bin/cucumber-js
...
239 scenarios (239 passed)
1201 steps (1201 passed)
0m25.237s
       38.10 real        23.52 user         3.50 sys

Timing still seems a bit off though as the timestamps in messages goes up to 34 (I'm guessing difference between pure user code and some cucumber overhead but not sure)

@davidjgoss
Copy link
Contributor Author

Will hit merge on this in the morning if no other issues

Copy link
Member

@charlierudolph charlierudolph left a comment

Choose a reason for hiding this comment

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

Woohoo! Thanks for all your work on this @davidjgoss

@davidjgoss davidjgoss merged commit fef94dd into master Aug 24, 2020
@davidjgoss davidjgoss deleted the cck branch August 24, 2020 06:39
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.

Emit a Meta message at the beginning of the run.
3 participants