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

Upgrade Spek to v2.0.8 #1992

Merged
merged 4 commits into from
Oct 12, 2019
Merged

Upgrade Spek to v2.0.8 #1992

merged 4 commits into from
Oct 12, 2019

Conversation

schalkms
Copy link
Member

@schalkms schalkms commented Oct 6, 2019

No description provided.

@3flex
Copy link
Member

3flex commented Oct 6, 2019

I'm guessing the failure is due to a behaviour change in Spek meaning described nested in describe fails where it didn't before.

Changing describe to context here should fix it: https://github.com/arturbosch/detekt/blob/1c93b1e9def6054fb17c26c7655ff31ad9bcda7a/detekt-cli/src/test/kotlin/io/gitlab/arturbosch/detekt/cli/console/FileBasedFindingsReportSpec.kt#L16

@schalkms
Copy link
Member Author

schalkms commented Oct 6, 2019

@3flex I'm on it. That's only a part of the problem.

'subject' can not be accessed in this context.
java.lang.AssertionError: 'subject' can not be accessed in this context.
@codecov-io
Copy link

codecov-io commented Oct 6, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@1c93b1e). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##             master   #1992   +/-   ##
========================================
  Coverage          ?   80.7%           
  Complexity        ?    1979           
========================================
  Files             ?     329           
  Lines             ?    5577           
  Branches          ?    1021           
========================================
  Hits              ?    4501           
  Misses            ?     538           
  Partials          ?     538

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1c93b1e...521b030. Read the comment docs.

@@ -35,7 +35,7 @@ class FileBasedFindingsReportSpec : Spek({
Pair("EmptySmells", emptyList())
)
}
val output = subject.render(detektion)?.trimEnd()?.decolorized()
val output by memoized { subject.render(detektion)?.trimEnd()?.decolorized() }
Copy link
Member

Choose a reason for hiding this comment

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

I think it's better to put this line in the it() context... the Spek docs say not to use memoized for holding the result of an action https://www.spekframework.org/core-concepts/#scope-values

So instead:

            it("has the reference content") {
                val output = subject.render(detektion)?.trimEnd()?.decolorized()
                assertThat(output).isEqualTo(expectedContent)

Will that work?

Copy link
Member Author

@schalkms schalkms Oct 6, 2019

Choose a reason for hiding this comment

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

With the updated commit we lint/compileAndLint the code for each and every test case, which is not really what we want.
BeforeEachGroup also doesn't work.

Copy link
Member

Choose a reason for hiding this comment

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

Another option is putting all assertions into a single test case for the tests that assert multiple things for each test file.

I think what you have here now though is ok. This problem will go away when test cases are moved out of the external files and into individual test cases.

The tests don't use memoized for holding the result of subject.lint().
Instead the tests use a fixture for holding the linted code.
https://www.spekframework.org/core-concepts/#scope-values
@3flex 3flex merged commit b048c6b into detekt:master Oct 12, 2019
@arturbosch arturbosch added this to the 1.2.0 milestone Oct 12, 2019
@schalkms schalkms deleted the spek_2-0-8 branch November 8, 2019 18:16
smyachenkov pushed a commit to smyachenkov/detekt that referenced this pull request Dec 9, 2019
* Upgrade Spek to v2.0.8

* Disable Spek timeouts

* Fix Spek exceptions with by memoized clause

'subject' can not be accessed in this context.
java.lang.AssertionError: 'subject' can not be accessed in this context.

* Use a Spek fixture for holding the result

The tests don't use memoized for holding the result of subject.lint().
Instead the tests use a fixture for holding the linted code.
https://www.spekframework.org/core-concepts/#scope-values
smyachenkov pushed a commit to smyachenkov/detekt that referenced this pull request Dec 9, 2019
* Upgrade Spek to v2.0.8

* Disable Spek timeouts

* Fix Spek exceptions with by memoized clause

'subject' can not be accessed in this context.
java.lang.AssertionError: 'subject' can not be accessed in this context.

* Use a Spek fixture for holding the result

The tests don't use memoized for holding the result of subject.lint().
Instead the tests use a fixture for holding the linted code.
https://www.spekframework.org/core-concepts/#scope-values
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants