-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Cucumber now creates whatever directories are necessary to make a report #2266
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, couple of requests.
…if it can't write output.
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. |
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. |
Not removing the try/catch, but reducing its scope. Reverting 26212c4 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. |
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. |
Thanks, I think going back to cf5d171 is the right thing for now |
Ok. Done (Well, I used CTRL-Z in Visual Studio code to rewind back to that point instead of git) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this!
🤔 What's changed?
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?
♻️ 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: