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

Fix compareCustom function in IrLineNumberTest class, to exclude comment #2321

Merged
merged 1 commit into from
May 10, 2019

Conversation

neetopia
Copy link
Contributor

@neetopia neetopia commented May 8, 2019

Fix compareCustom function in IrLineNumberTest class, to exclude comment lines that are not related to line number verification.

Unmute a test case should have been unmuted but shadowed by this bug.

fileText.substring(fileText.indexOf("//") + 2)
.trim().split(" ").map { it.trim() }.toMutableList()
)
val expectedLineNumbers = fileText.split("\n".toRegex()).filter { line ->
Copy link
Contributor

Choose a reason for hiding this comment

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

How about simplifying this to:

    val expectedLineNumbers = normalize(
        fileText.substring(Regex("// \\d+").find(fileText)!!.range.start + 2)
            .trim().split(" ").map { it.trim() }.toMutableList()
    )

Then we are looking for a line that starts with "// " with a number of digits after it. That should work to exclude the comment lines with text. That looks consistent with what the old backend expects from these tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks reasonable since this is how old test is doing. I was a little bit overthinking this, was considering the case where there is a trailing comment line after line numbers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am also considering adding an optional "+" check before digit check, since there are line numbers start with a "+", although such case is not present in current test data yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

It wouldn't presented in future (answered in slack)

lines that are not related for line number verification.

Unmute a test case should have been unmuted but shadowed by this bug.
@neetopia neetopia force-pushed the irLineNumberTestFix branch from 8a8e9e5 to 2c5ebf3 Compare May 8, 2019 19:54
@neetopia neetopia marked this pull request as ready for review May 9, 2019 07:26
@max-kammerer max-kammerer merged commit eeb1c0a into JetBrains:master May 10, 2019
@max-kammerer
Copy link
Contributor

Thanks!

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.

3 participants