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

Make Scapegoat sensor work with absolute file paths in the Scapegoat report #116

Merged
merged 4 commits into from Sep 28, 2018

Conversation

@mwz
Copy link
Owner

mwz commented Sep 25, 2018

I'm missing some unit tests with absolute file paths in the test Scapegoat reports, but I'll add those later if you're happy with this fix @BalmungSan.

@samlin425 you might also be interested in having a look at this.

Fixes #115.

val scapegoatPath = "com.mwz.sonar.scala.scapegoat.TestFile.scala"
val linuxPath = scapegoatReportParser.replaceAllDotsButLastWithSlashes(scapegoatPath)

linuxPath shouldBe "com/mwz/sonar/scala/scapegoat/TestFile.scala"
}

it should "replace all dots (.) with slashes (/) in a scapegoat absolute path except for the file extension" in {
val scapegoatPath = cwd.resolve("ScapegoatReportParserSpec.scala").toString.replace("/", ".")

This comment has been minimized.

Copy link
@BalmungSan

BalmungSan Sep 25, 2018

Contributor

I would rather use a literal value. Both for the input and for the expected result.

This comment has been minimized.

Copy link
@mwz

mwz Sep 25, 2018

Author Owner

I don't think this is possible because the new implementation tests for the existence of the path on the file system so the input and output are both dynamic values based on where this project is located in the file system.

This comment has been minimized.

Copy link
@BalmungSan

BalmungSan Sep 25, 2018

Contributor

Oh, yeah yeah my fault...


linuxPath shouldBe cwd.resolve("ScapegoatReportParserSpec.scala").toString
}

This comment has been minimized.

Copy link
@BalmungSan

BalmungSan Sep 25, 2018

Contributor

I would also add a test with filenames that have dots (.) inside them. Like: target/scala.12/.file.Scala.

private[scapegoat] def replaceAllDotsButLastWithSlashes(path: String): String =
AllDotsButLastRegex.replaceAllIn(target = path, replacement = "/")
if (path.startsWith(".")) {

This comment has been minimized.

Copy link
@BalmungSan

BalmungSan Sep 25, 2018

Contributor

I can't confirm it, neither test it (cause I'm on my cell phone right now).
But, I think this isn't working the way you think.
For example, I think this file will make it fail: target.scala.12..file.Scala

Ok, so I just noticed that all files in #115 scapegoat report start with '.' - that's probably because they are full paths. But, that wasn't the problem you tried to solve here, but instead paths with dots in their files - which could be relative.

This comment has been minimized.

Copy link
@mwz

mwz Sep 25, 2018

Author Owner

I'll add some tests with extra dots in the middle of the path, but I think it's rather uncommon to have Scala source files hidden, i.e. starting with a ., so I think we can safely ignore this case.

This comment has been minimized.

Copy link
@BalmungSan

BalmungSan Sep 25, 2018

Contributor

Hidden files probably don't, but what about paths like scala-2.12 ?

This comment has been minimized.

Copy link
@mwz

mwz Sep 25, 2018

Author Owner

yeah, so your example would work since the implementation checks first if scala-2 exists in the file system and if it doesn't instead of resolving 12 as a subdirectory it concatenates it with a . and the previous path before checking again for existence in the file system - I'll add a unit test to cover that.

@mwz

This comment has been minimized.

Copy link
Owner Author

mwz commented Sep 27, 2018

I've pushed unit tests, @BalmungSan would you mind having another look?

linuxPath shouldBe file.toString
}

it should "handle correctly multiple dots in path" in withFile(cwd.resolve("example.file.scala")) { file =>

This comment has been minimized.

Copy link
@BalmungSan

BalmungSan Sep 28, 2018

Contributor

Aren't those two tests exactly the same?, or what am I missing?

This comment has been minimized.

Copy link
@mwz

mwz Sep 28, 2018

Author Owner

Sorry, it was meant to be a different test - I fixed it in 9c8fdba.

This comment has been minimized.

Copy link
@BalmungSan

BalmungSan Sep 28, 2018

Contributor

👍

Copy link
Contributor

BalmungSan left a comment

👍

@mwz mwz merged commit b8831b3 into master Sep 28, 2018
1 check passed
1 check passed
ci/circleci: test Your tests passed on CircleCI!
Details
@mwz mwz deleted the scapegoat-absolute-file-paths branch Sep 28, 2018
@samlin425

This comment has been minimized.

Copy link

samlin425 commented Oct 22, 2018

👍

2 similar comments
@samlin425

This comment has been minimized.

Copy link

samlin425 commented Oct 22, 2018

👍

@samlin425

This comment has been minimized.

Copy link

samlin425 commented Oct 22, 2018

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.