This repository has been archived by the owner on Nov 30, 2024. It is now read-only.
PR 2368 rebased #2379
Merged
PR 2368 rebased #2379
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 3 commits
January 30, 2017 18:42
This was prompted by #2367 and the cumulative reporting out of deprecations. Semantically, this could be weird! Should clearing examples also clear the reporter? If the reporter is configured after initialization through a means other than standard RSpec.configure, that configuration would be lost. I don't know why you would do that though? The original intent of clear_examples (from #1700) seems to imply that reseting the reporters was desirable, so I think this is OK. It's weird that reporter belongs to configuration. It mixes state with configuration. This means that we have to do a "partial" configuration reset when we want to "reset except configuration", which suggests to me that we'll probably have more issues here in the future. Reported (among other things) in #2367
While technically examples, shared ones function more like configuration. When using clear_examples, it is surprising that they are removed since a shared library of them was likely loaded in the initial configuration steps. This is a change in behaviour. It was initially added in 2.9 in 8e8fb2b with no rationale. Its API visibility was unspecified at the time, and it's been officially private since 3. Reported (among other things) in #2367.
RSpec.configure { |c| c.output_stream = File.open('testout', 'w') }
RSpec::Core::Runner.run(["spec1"])
RSpec::Core::Runner.run(["spec2"])
This implementation is possibly a can of worms, but I _think_ I know
what I'm doing?
* close does not guarantee anymore durability than flush. To ensure "no
bytes are lost" an fsync would be needed. Not tackling that here,
instead just removing the relevant spec since it wasn't valid.
* BaseFormatter contains logic to save and restore sync, which is broken
for closed streams. The restore method was patched in the past, but
the start method wasn't. Also, these methods aren't reliably called
since BaseTextFormatter doesn't call super. This all probably needs a
rethink.
* The one reason to close would be to ensure that we don't leak file
handles. The only time this could cause a problem is if the process
did a heap of work _after_ rspec was finished running but before
exiting (or in our specs if we're not careful!). In that case, since
it's likely the object would still have an active reference via global
configuration, it would stay open. I think this is a weird enough
case, and small enough impact, that we're better off just letting
process exit or GC close the file. It's also somewhat expected, since
that's what you typically get with a non-block File.open.
* We don't close the deprecation stream anyway. And also it's weird to
check for $stdout to avoid closig that, but not (say) $stderr.
* close is probably a misleading name for this formatter API, but it's
public :( Any existing third-party formatters that close streams in
this method today will be broken in this reuse case anyway though.
Reported (among other things) in #2367
|
LGTM, merging because the build failure is just JRuby failing to install rubocop, see #2380 |
MatheusRich
pushed a commit
to MatheusRich/rspec-core
that referenced
this pull request
Oct 30, 2020
PR 2368 rebased
yujinakayama
pushed a commit
to yujinakayama/rspec-monorepo
that referenced
this pull request
Oct 6, 2021
…368-rebased PR 2368 rebased --- This commit was imported from rspec/rspec-core@450552a.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is #2368 rebased so the build will hopefully go green, and then we can merge.
/cc @xaviershay @JonRowe