Cucumber now creates whatever directories are necessary to make a report

michael-lloyd-morris commented 1 year ago

🤔 What's changed?

  Scenario: Created relative path
    When I run cucumber-js with `-f summary:some/long/path/for/reports/summary.txt`
    Then the file "some/long/path/for/reports/summary.txt" has the text:
      """
      1 scenario (1 passed)
      1 step (1 passed)
      <duration-stat>
      """

Previously Cucumber wouldn't created long directory chains as seen in this feature - some outside process had to make sure the directory the report file was to be written into exists.

⚡️ What's your motivation?

Fixes #2159

🏷️ What kind of change is this?

:zap: New feature (non-breaking change which adds new behaviour)

♻️ Anything particular you want feedback on?

There was a test in place to see if Cucumber will fail if the directory didn't exist that was removed. It is possible for this feature to fail if Cucumber lacks permission to create directories - but I feel testing this is a bit of overkill as such permission checking wasn't previously being tested and in any event it's a feature of the OS, not Cucumber.

📋 Checklist:

[x] I agree to respect and uphold the Cucumber Community Code of Conduct
[x] I've changed the behaviour of the code
- [x] I have added/updated tests to cover my changes.
[x] My change requires a change to the documentation.
- [x] I have updated the documentation accordingly.
[x] Users should know about my change
- [x] I have added an entry to the "Unreleased" section of the CHANGELOG, linking to this pull request.

coveralls commented 1 year ago

Coverage: 98.525% (-0.04%) from 98.561% when pulling 8a0a36c596b30c265342d7fbec06a092bca632e1 on michael-lloyd-morris:issue2159 into 627030d4d657b7efc19b1343d3d77d77c774139e on cucumber:main.

davidjgoss commented 1 year ago

I'd like to merge this but after reverting the last commit. It expands the scope of errors we'll tolerate to things like the formatter class failing to load, or an error in its constructor etc, which isn't really about the target dir/file any more.

michael-lloyd-morris commented 1 year ago

Well, if I remove the try/catch block then we return to the prior behavior of Cucumber halting with an ENOT error if the file can't be created. Shall I do that?

Logging a warning doesn't really help us. If mkdirp doesn't make the directory the program will crash with ENOT when it tries to open the write stream. We could also catch and throw an error detailing that the file couldn't be created - this will be more in line with existing behavior. I also think it's the safest since running a test without a report detail is as bad as not running it at all, especially if the test takes a long while to run.

davidjgoss commented 1 year ago

Not removing the try/catch, but reducing its scope. Reverting https://github.com/cucumber/cucumber-js/pull/2266/commits/26212c4c57841d65cf5359ce06e6e63538c8cdf7 would get it back down to just the attempt to ensure the directory i.e. the new behaviour - really as a guard in case that has unexpected effects on an otherwise working setup. That feels like the right step for this PR. I'm not sure if failing to create the stream for a formatter should be downgraded to a warning at this point.

michael-lloyd-morris commented 1 year ago

I'll change to immediate die so you can see what that looks like. If that isn't good I'll do a full revert back to the point you were comfortable with.

davidjgoss commented 1 year ago

Thanks, I think going back to https://github.com/cucumber/cucumber-js/pull/2266/commits/cf5d171757bc05ec0b24360e6f092fa98dd70dfb is the right thing for now

michael-lloyd-morris commented 1 year ago

Ok. Done (Well, I used CTRL-Z in Visual Studio code to rewind back to that point instead of git)

cucumber / cucumber-js