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 bdd tests for jrnl installation #1513

Merged
merged 2 commits into from
Jun 25, 2022

Conversation

apainintheneck
Copy link
Contributor

Checklist

  • I have read the contributing doc.
  • I have included a link to the relevant issue number.
  • I have checked to ensure there aren't other open pull requests
    for the same issue.
  • I have written new tests for these changes, as needed.

This is just a first stab at adding BDD tests related to installation as mentioned in #1473. I couldn't really find a way to make it work with just Gherkin because of some of the required mocking and steps that were added. Is that sorta what you had in mind?

The biggest change I made was to use the pipe character | as the equivalent of an empty line of input in the all_inputs fixture used by the when_steps.we_run_jrnl() method. This change made it a lot easier to test the default journal installation case and I think it makes the BDD test more readable too. Do you all know of a better way to handle that?

Resolves #1473.

Comment on lines 6 to 7
|
|
Copy link
Member

Choose a reason for hiding this comment

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

Could you use \n for this instead of a pipe? I think that will be a bit more Gherkin-appropriate since the Gherkin reference has some allowances for this inside of table cells already.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I can do that. I can see how that might be confusing.

@wren
Copy link
Member

wren commented Jun 18, 2022

@apainintheneck Hey, this PR is looking good so far; we just need a little more time to review (had a lot of PRs today, and focusing on the beta). We'll come back to this next week and get merged in. Thank you!

Copy link
Member

@micahellison micahellison left a comment

Choose a reason for hiding this comment

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

Just one tiny case change, please. Thanks for adding these much-needed tests.

@micahellison micahellison added the build Issues related to the build pipeline label Jun 25, 2022
@micahellison micahellison changed the title Added bdd tests for jrnl installation Add bdd tests for jrnl installation Jun 25, 2022
@micahellison micahellison merged commit 8b955ef into jrnl-org:develop Jun 25, 2022
@apainintheneck apainintheneck deleted the install-tests branch June 28, 2022 05:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues related to the build pipeline
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add installation BDD tests
3 participants