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

Report Evaluation Result After notToThrow() #628

Closed
jGleitz opened this issue Oct 12, 2020 · 11 comments · Fixed by #688
Closed

Report Evaluation Result After notToThrow() #628

jGleitz opened this issue Oct 12, 2020 · 11 comments · Fixed by #688
Assignees
Milestone

Comments

@jGleitz
Copy link
Collaborator

jGleitz commented Oct 12, 2020

Platform (all, jvm, js, android): all
Extension (none, kotlin 1.3): none

Code related feature

expect { "test value" }.notToThrow().contains("42")

leads to the output:

expected that subject: () -> kotlin.String        (DefaultTestFilesSpec$1$2$4$4$1 <604188911>)
◆ contains: 
  ⚬ value: "42"        <1804862564>
    ⚬ ▶ number of matches: 0
        ◾ is at least: 1

which does not show me the result of evaluating the subject.

Describe the solution you'd like
The subject after notToThrow() should be reported. Something like:

expected that subject: () -> kotlin.String        (DefaultTestFilesSpec$1$2$4$4$1 <604188911>)
⚬evaluated: "test value"            (…)
  ◆ contains: 
    ⚬ value: "42"        <1804862564>
      ⚬ ▶ number of matches: 0
          ◾ is at least: 1

Describe alternatives you've considered
none

@robstoll
Copy link
Owner

Good input and I agree. I would just use invoked instead of evaluated.

@sharibj
Copy link

sharibj commented Oct 16, 2020

I'm a new contributor.
I'll work on this.
Feel free to direct me to some more issues a beginner can take.

@robstoll
Copy link
Owner

Great! This one isn't a good first issue though, but you can give it a try. I don't know it by heart but the change is most likely in DefaultFun0Assertions. We probably use changeSubject but should use extractFeature in order to get the reporting we want

@sharibj
Copy link

sharibj commented Oct 16, 2020

Thanks for the tips.
This is my very first attempt and I've just started going through the code so I'm sure the tips will help.
Do let me know if you know a 'good first issue' which is not taken yet.

@robstoll
Copy link
Owner

@sharibj I created a bunch of good first issues which are fairly easy in case you would like to switch

@robstoll
Copy link
Owner

robstoll commented Nov 1, 2020

@sharibj unassigning you so that others can take over

@robstoll robstoll assigned robstoll and unassigned sharibj Nov 1, 2020
@robstoll robstoll added this to the 0.14.0 milestone Nov 3, 2020
@robstoll
Copy link
Owner

robstoll commented Nov 9, 2020

@jGleitz I would be interested on your opinion. I am going to change feature exceptions in general with this change so that they catch exceptions in case feature extraction fails and shows them, like we already did for notToThrow. notToThrow will then just be an alias for invoke(). What would you expect: seeing the sub-assertions you defined for the feature first and then the properties of the unexpected exception or first the properties:
For instance, for:

expect<() -> Int> {
    /...
}.notToThrowFun { toBe(2) }

the following report

expected that subject: () -> kotlin.Nothing        (ch.tutteli.atrium.specs.integration.Fun0AssertionsSpec$1$17$2$$special$$inlined$forEach$lambda$2$1$1 <1662198177>)
◆ ▶ invoke(): ❗❗ threw java.lang.UnsupportedOperationException
      » equals: 2        (kotlin.Int <1997154602>)
      ℹ️ Properties of the unexpected UnsupportedOperationException
        » message: "oho... error occurred"        <2030822440>
        » stacktrace: 
          ⚬ ch.tutteli.atrium.specs.integration.Fun0AssertionsSpec$1$17$2.invoke(Fun0AssertionsSpec.kt:107)
          ⚬ ch.tutteli.atrium.specs.integration.Fun0AssertionsSpec$1$17$2.invoke(Fun0AssertionsSpec.kt:15)
          ⚬ org.spekframework.spek2.style.specification.SpecificationStyleKt$createSuite$1.invoke(specificationStyle.kt:88)
          ⚬ org.spekframework.spek2.style.specification.SpecificationStyleKt$createSuite$1.invoke(specificationStyle.kt)
          ⚬ org.spekframework.spek2.runtime.Collector.group(Collectors.kt:91)
          ⚬ org.spekframework.spek2.dsl.GroupBody$DefaultImpls.group$default(dsl.kt:24)
          ⚬ org.spekframework.spek2.style.specification.SpecificationStyleKt.createSuite(specificationStyle.kt:87)
          ⚬ org.spekframework.spek2.style.specification.SpecificationStyleKt.access$createSuite(specificationStyle.kt:1)
          ⚬ org.spekframework.spek2.style.specification.Suite.context(specificationStyle.kt:26)
          ⚬ org.spekframework.spek2.style.specification.Suite.context$default(specificationStyle.kt:25)
          ⚬ ch.tutteli.atrium.specs.integration.Fun0AssertionsSpec$1$17.invoke(Fun0AssertionsSpec.kt:105)
          ⚬ ch.tutteli.atrium.specs.integration.Fun0AssertionsSpec$1$17.invoke(Fun0AssertionsSpec.kt:15)

vs

expected that subject: () -> kotlin.Nothing        (ch.tutteli.atrium.specs.integration.Fun0AssertionsSpec$1$17$2$$special$$inlined$forEach$lambda$2$1$1 <1662198177>)
◆ ▶ invoke(): ❗❗ threw java.lang.UnsupportedOperationException
      ℹ️ Properties of the unexpected UnsupportedOperationException
        » message: "oho... error occurred"        <2030822440>
        » stacktrace: 
          ⚬ ch.tutteli.atrium.specs.integration.Fun0AssertionsSpec$1$17$2.invoke(Fun0AssertionsSpec.kt:107)
          ⚬ ch.tutteli.atrium.specs.integration.Fun0AssertionsSpec$1$17$2.invoke(Fun0AssertionsSpec.kt:15)
          ⚬ org.spekframework.spek2.style.specification.SpecificationStyleKt$createSuite$1.invoke(specificationStyle.kt:88)
          ⚬ org.spekframework.spek2.style.specification.SpecificationStyleKt$createSuite$1.invoke(specificationStyle.kt)
          ⚬ org.spekframework.spek2.runtime.Collector.group(Collectors.kt:91)
          ⚬ org.spekframework.spek2.dsl.GroupBody$DefaultImpls.group$default(dsl.kt:24)
          ⚬ org.spekframework.spek2.style.specification.SpecificationStyleKt.createSuite(specificationStyle.kt:87)
          ⚬ org.spekframework.spek2.style.specification.SpecificationStyleKt.access$createSuite(specificationStyle.kt:1)
          ⚬ org.spekframework.spek2.style.specification.Suite.context(specificationStyle.kt:26)
          ⚬ org.spekframework.spek2.style.specification.Suite.context$default(specificationStyle.kt:25)
          ⚬ ch.tutteli.atrium.specs.integration.Fun0AssertionsSpec$1$17.invoke(Fun0AssertionsSpec.kt:105)
          ⚬ ch.tutteli.atrium.specs.integration.Fun0AssertionsSpec$1$17.invoke(Fun0AssertionsSpec.kt:15)
      » equals: 2        (kotlin.Int <1997154602>)

@robstoll
Copy link
Owner

robstoll commented Nov 9, 2020

Many times it really helps to just ask someone (even if virtual). I prefer the first solution. Convince me otherwise ?

robstoll added a commit that referenced this issue Nov 9, 2020
- re-use this for notToThrow, change to feature extraction as we want
  to show the value in case of a success (fixes #628)
- remove genericFeature and genericSubjectBasedFeature based on
  MetaFeature from logic
robstoll added a commit that referenced this issue Nov 9, 2020
- re-use this for notToThrow, change to feature extraction as we want
  to show the value in case of a success (fixes #628)
- remove genericFeature and genericSubjectBasedFeature based on
  MetaFeature from logic
@jGleitz
Copy link
Collaborator Author

jGleitz commented Nov 10, 2020

Convince me otherwise

Challenge accepted! I prefer the second solution for two reasons:

  1. When looking at failed tests, I want to know what went wrong as quickly as possible. I am in a very selective mode of perception and anything that leads me to the error is helpful. So seeing the exception’s message and stacktrace as soon as possible is great.
  2. The first solution jumps topics, which makes it harder to understand. First we talk about a failure, then of something unrelated (namely what we would have checked if the failure had not occured), then about the failure again.

@jGleitz
Copy link
Collaborator Author

jGleitz commented Nov 10, 2020

Many times it really helps to just ask someone (even if virtual).

I am always glad to be your sparring partners for design discussions. Even if it is just rubber duck design debugging :)

@robstoll
Copy link
Owner

Convince me otherwise

Challenge accepted! I prefer the second solution for two reasons:

1. When looking at failed tests, I want to know what went wrong as quickly as possible. I am in a very selective mode of perception and anything that leads me to the error is helpful. So seeing the exception’s message and stacktrace as soon as possible is great.

2. The first solution jumps topics, which makes it harder to understand. First we talk about a failure, then of something unrelated (namely what we would have checked if the failure had not occured), then about the failure again.

I see your point, I had the same thoughts kind of but just the opposite: I want too see as quickly as possible which assertion failed. So in case my test has multiple notToThrow assertions then seeing the context first is more helpful than the stacktrace. On the other hand, if we see the stacktrace first, we also know immediately where to look (and if we run a test in intellij we can even jump) and I agree that the context switch in the first solution is not so nice.
To sum up, you convinced me, let's go with the second solution 👍

robstoll added a commit that referenced this issue Nov 12, 2020
- re-use this for notToThrow, change to feature extraction as we want
  to show the value in case of a success (fixes #628)
- remove genericFeature and genericSubjectBasedFeature based on
  MetaFeature from logic
robstoll added a commit that referenced this issue Nov 12, 2020
- re-use this for notToThrow, change to feature extraction as we want
  to show the value in case of a success (fixes #628)
- remove genericFeature and genericSubjectBasedFeature based on
  MetaFeature from logic
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants