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

Allow absolute formatter output paths. Fixes #900. #906

Merged
merged 1 commit into from
Aug 23, 2017
Merged

Allow absolute formatter output paths. Fixes #900. #906

merged 1 commit into from
Aug 23, 2017

Conversation

darrinholst
Copy link
Contributor

@darrinholst darrinholst commented Aug 17, 2017

I needed this fix to get protractor support for cucumber 3.

  • Moved the invalid path scenario that was in multiple_formattters.feature to a new formatter_paths.feature so all the path related scenarios were together.
  • Added mustache to get the temp dir into the feature. Added a new replaceContextVariables helper so that I could get the actual temp dir into the feature itself. It felt kind of hacky, but I didn't know of another way besides duplicating step definitions to deal with absolute paths explicitly.

"""

Scenario: Absolute path
When I run cucumber.js with `-f summary:{this.tmpDir}/summary.txt`
Copy link
Member

Choose a reason for hiding this comment

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

Thoughts on using https://github.com/janl/mustache.js/ for the templating?

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...if there were only templating libraries that already handled string replacements 😄. I'll look into this afternoon.

"""

Scenario: Absolute path
When I run cucumber.js with `-f summary:{{{tmpDir}}}/summary.txt`
Copy link
Member

Choose a reason for hiding this comment

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

Why are there three curly braces? In the mustache documentation it appears you only need two.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mustache html escapes by default which was escaping the /. Triple { returns the raw value.

Copy link
Member

Choose a reason for hiding this comment

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

Cool. Thanks for the explanation


Scenario: Absolute path
When I run cucumber.js with `-f summary:{{{tmpDir}}}/summary.txt`
Then the file "{{{tmpDir}}}/summary.txt" is absolute
Copy link
Member

Choose a reason for hiding this comment

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

I don't like this step. It appears to only be testing that we are using mustache templates correctly. If we remove this step, does the test still fail if we revert the source code change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My thinking there was that I don't have any control over what tmpDir is. It could be relative to the working directory for all I know so it was just some extra assurance that we are really fixing the problem.

Yes, the test will still fail if it's removed...as long as tmpDir is and always continues to be an absolute path.

I can remove it if you want.

Copy link
Member

Choose a reason for hiding this comment

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

Okay. That sounds like an important precondition. Thoughts on using a 'Given' step instead? Maybe one that says: 'Given "{{{tmpDir}}}" is an absolute path'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds reasonable

@charlierudolph charlierudolph merged commit e306c83 into cucumber:master Aug 23, 2017
@aslakhellesoy
Copy link
Contributor

Hi @darrinholst,

Thanks for your making your first contribution to Cucumber, and welcome to the Cucumber committers team! You can now push directly to this repo and all other repos under the cucumber organization! 🍾

In return for this generous offer we hope you will:

  • ✅ Continue to use branches and pull requests. When someone on the core team approves a pull request (yours or someone else's), you're welcome to merge it yourself.
  • 💚 Commit to setting a good example by following and upholding our code of conduct in your interactions with other collaborators and users.
  • 💬 Join the community Slack channel to meet the rest of the team and make yourself at home.
  • ℹ️ Don't feel obliged to help, just do what you can if you have the time and the energy.
  • 🙋 Ask if you need anything. We're looking for feedback about how to make the project more welcoming, so please tell us!

On behalf of the Cucumber core team,
Aslak Hellesøy
Creator of Cucumber

@lock
Copy link

lock bot commented Oct 24, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Oct 24, 2018
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.

3 participants