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

Set sorting reporter as complete if test run failed unexpectedly #2161

Merged
merged 1 commit into from Sep 23, 2022

Conversation

nimatrueway
Copy link
Contributor

@nimatrueway nimatrueway commented Aug 21, 2022

If test runner reporter is of "sorted" type, it buffers all events and eventually emits/prints them upon completion of the test. Now, in case test runner fails unexpectedly it does not flush buffered events until an internal timer kicks off. This won't work with SBT system as all loggers of a task are already discarded when a task is finished.

Sample

class BugBefore extends wordspec.AsyncWordSpec with BeforeAndAfter {

  before {
    fail("BeforeEach failure.")
  }

  "BeforeAndAfter" should {
    "print full stack trace if failed at 'before' stage." in {
      Future {
        succeed
      }
    }
  }
}

Before this PR
scalatest-before

After this PR
New Project

@cla-bot
Copy link

cla-bot bot commented Aug 21, 2022

Hi @nimataheri-hootsuite, we require contributors to sign our Contributor License Agreement, and we don't have yours on file. In order for us to review and merge your code, please access https://www.artima.com/cla/choose-type to sign our Contributor License Agreement. Your effort is highly appreciated. Thank you.

@nimatrueway nimatrueway changed the title Set sorting reporter as complete if test run failed Set sorting reporter as complete if test run failed unexpectedly Aug 21, 2022
If test runner is of sorted type, it buffers all events and eventually emits/prints them upon completion of the test. Now, in case test runner fails unexpectedly it does not flush buffered events until an internal timer kicks off; this won't work with SBT system as all loggers of a task are discarded by the time that internal timer is triggered.
@cheeseng
Copy link
Contributor

@nimatrueway This PR looks good to me, thanks!

@bvenners Unfortunately this PR contains a lot of whitespace diff, in order to filter out whitespace diff lines you may apply the filter like this:

Screenshot from 2022-08-22 22-55-05

@nimatrueway
Copy link
Contributor Author

@cheeseng thanks for reviewing it! I can remove all whitespace changes if it helps but I figured this formatting will happen eventually.

case e: Throwable => {
suiteSortingReporter.completedTests(suite.suiteId)

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we call this again after the timer expires. It sounds like it may not matter with running with sbt, but I wonder if that causes issues when running not with sbt.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey Bill!

completedTests method invokes fireReadyEvents which in turn calls cancelTimeoutTask to cancel the subject timer. Whether SuiteSortingReporter#timeout task is thread-safe (in a race with completedTests) is a more difficult question which I'm afraid I can't answer. However, that could happen regardless of this PR's change I believe. Namely, a test suite completes in the happy code path and simultaneously SuiteSortingReporter#timeout task gets fired after 2 sec.

Copy link
Contributor

Choose a reason for hiding this comment

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

@bvenners @nimatrueway Framework.scala is called by sbt only, anyway, that also raises a question if we should do the similar when run with Runner, for Runner I think the similar spot is in SuiteRunner.scala:

https://github.com/scalatest/scalatest/blob/main/jvm/core/src/main/scala/org/scalatest/tools/SuiteRunner.scala#L79

I am not sure how we can call completedTests there, we probably shouldn't. Different from the sbt runner, the ScalaTest Runner will wait until the timeout and got the SuiteAborted eventually, I think.

@cheeseng cheeseng merged commit 3e83214 into scalatest:main Sep 23, 2022
@nimatrueway nimatrueway deleted the complete-reporter-if-failed branch September 23, 2022 08:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants