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

Redesign (#1) #2

Open
wants to merge 87 commits into
base: main
Choose a base branch
from
Open

Redesign (#1) #2

wants to merge 87 commits into from

Conversation

ilslv
Copy link
Owner

@ilslv ilslv commented Jul 23, 2021

Part of #1

Synopsis

Provide new Parser, Runner and Writer abstractions and tools to leverage them.

Solution

  • Parser trait
    • parser::Basic impl
  • Runner trait
    • runner::Basic impl
  • Writer trait
    • writer::Basic impl
    • writer::Summary impl
    • writer::Normalize impl
    • WriterExt trait
  • Update cucumber_rust_codegen

Checklist

  • Created PR:
    • In draft mode
    • Name contains Draft: prefix
    • Name contains issue reference
    • Has k:: labels applied
    • Has assignee
  • Documentation is updated (if required)
  • Tests are updated (if required)
  • Changes conform code style
  • CHANGELOG entry is added (if required)
  • FCM (final commit message) is posted
    • and approved
  • Review is completed and changes are approved
  • Before merge:
    • Milestone is set
    • PR's name and description are correct and up-to-date
    • Draft: prefix is removed
    • All temporary labels are removed

@ilslv ilslv added the k::design Related to overall design and/or architecture label Jul 23, 2021
@ilslv ilslv self-assigned this Jul 23, 2021
@ilslv ilslv added k::refactor Refactoring, technical debt elimination and other improvements of existing code base enhancement New feature or request labels Jul 23, 2021
@ilslv ilslv mentioned this pull request Jul 23, 2021
3 tasks
@ilslv
Copy link
Owner Author

ilslv commented Jul 23, 2021

FCM

Redesign (#1, #2)

- introduce `Parser` trait and `parser::Basic` implementation
- introduce `Runner` trait and `runner::Basic` implementation 
- introduce `Writer` trait and `writer::Basic`, `writer::Summary`, `writer::Normalized` implementations

Additionally:
- update book and add CI testing and deployment for it

Book link

Resolved Issues

  • 126 — uses # language: comment to deduce language
  • 123writer::FailOnSkipped
  • 119writer::Basic pretty-prints only if terminal was detected
  • 103 — can be done by Cucumber::filter_run_and_exit()
  • 102Cucumber::language()
  • 88 — not applicable anymore
  • 86 — fixed
  • 71 — not applicable anymore
  • 80 — parsing errors are propagated to the Writer
  • 64 — can be done with regex
  • 63 — pretty well-documented now
  • 62asciinema demonstration added
  • 56writer::Basic has really minimal output

Not resolved yet, but provided more tools for it

  • 127 — can be done with custom Writer
  • 50 — can be done with custom Writer

TODO

@ilslv ilslv requested a review from tyranron July 26, 2021 12:54
@ilslv ilslv changed the title Draft: Initial redesign (#1) Draft: Redesign (#1) Jul 30, 2021
@ilslv ilslv mentioned this pull request Jul 30, 2021
19 tasks
@ilslv ilslv requested a review from tyranron September 8, 2021 06:36
@ilslv
Copy link
Owner Author

ilslv commented Sep 9, 2021

@tyranron also as described in the docs, Parser should override default language, if comment # language: present. Added that as part of this PR.

Corresponding issue in upstream: 126

Copy link

@tyranron tyranron left a comment

Choose a reason for hiding this comment

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

@ilslv this is finally approved! Don't merge it, however. This branch will go to upstream directly. I'll prepare the PR shortly.

Go ahead and integrate it to our projects.

@tyranron tyranron changed the title Draft: Redesign (#1) Redesign (#1) Sep 15, 2021
@tyranron tyranron marked this pull request as ready for review September 15, 2021 09:34
Copy link

@tyranron tyranron left a comment

Choose a reason for hiding this comment

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

@ilslv yet still found some style issues to be resolved:

  1. In the Book we need to start example steps from lowercase letter to comply with official Cucumber tutorials and don't spread unusual style in examples:

      Scenario: If we feed a hungry cat it will no longer be hungry
        Given a hungry cat
        When I feed the cat
        Then the cat is not hungry
  2. We've stripped out the file names and line numbers. But the original idea was to still keep them for failing tests, so the once could easily find the problematic .feature file. From the attached images in the Book I see that for failing tests they're stripped too. Please, return.

Unpleasant part is to recreate SVG images. 😥

@ilslv ilslv requested a review from tyranron September 15, 2021 12:06
Copy link

@tyranron tyranron left a comment

Choose a reason for hiding this comment

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

@ilslv

  1. Please, re-check and do update all the GIFs across project carefully. Only failed has been updated.

  2. In summary counts of failed/skipped/succeed should be highlighted with the correspondent color. Also, we should show statictics by scenarios, and skip rules/errors if there is none.
    Screenshot 2021-09-15 at 17 12 40
    It would be even better to preserve the previous style with bold text:
    Screenshot 2021-09-15 at 17 17 46

Do 1 after 2.

@ilslv
Copy link
Owner Author

ilslv commented Sep 16, 2021

@tyranron I've also separated FailOnSkipped Writer, as before that Summarized just failed in case of skipped step, but didn't show, which of them are failed skips or allowed. Now, FailOnSkipped alters events by itself, so skipped steps are only those, that were allowed.
Because of that, additional refactoring was done to separate writer::term::Styles and writer::Fallible

Copy link

@tyranron tyranron left a comment

Choose a reason for hiding this comment

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

@ilslv 👍

Just renamed writer::Fallible to writer::Failure, as the former confuses about being itself fallible rather than taliking about execution failures.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request k::design Related to overall design and/or architecture k::refactor Refactoring, technical debt elimination and other improvements of existing code base
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants