Skip to content

Adding unit test for parsing errors on frontend split queries #4504

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

Merged
merged 3 commits into from
Oct 6, 2021

Conversation

marianafranco
Copy link
Contributor

@marianafranco marianafranco commented Oct 1, 2021

Signed-off-by: Mariana Franco [email protected]

What this PR does:
Adds extra unit tests for the following fix: #4488

This will help to make sure 400s don't turn again to 500s.

Which issue(s) this PR fixes:
Fixes #

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

Signed-off-by: Mariana Franco <[email protected]>
@@ -349,3 +351,31 @@ func Test_evaluateAtModifier(t *testing.T) {
})
}
}

func Test_evaluateAtModifier_Error(t *testing.T) {
Copy link
Contributor

@alvinlin123 alvinlin123 Oct 1, 2021

Choose a reason for hiding this comment

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

My opinion is that we should merge this test into Test_evaluateAtModifier. We can extend Test_evaluateAtModifier's parameter to have an optional expectedErrorCode, then go from there.

The reason I think so is because this function seems quite duplicated to Test_evaluateAtModifier. It's also easier to discover what test cases are there for at-modifiers if we group them into one function.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense! I pushed a new commit with this change.

Copy link
Contributor

@alvinlin123 alvinlin123 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the new tests!

Copy link
Contributor

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

Thanks!

@pracucci pracucci merged commit a5a3c76 into cortexproject:master Oct 6, 2021
srijan55 pushed a commit to srijan55/cortex that referenced this pull request Nov 26, 2021
…project#4504)

* Adding unit test for parsing errors on frontend split queries

Signed-off-by: Mariana Franco <[email protected]>

* Fix lint errors

Signed-off-by: Mariana Franco <[email protected]>

* Merging error test cases with success ones

Signed-off-by: Mariana Franco <[email protected]>
Signed-off-by: Manish Kumar Gupta <[email protected]>
alvinlin123 pushed a commit to ac1214/cortex that referenced this pull request Jan 14, 2022
…project#4504)

* Adding unit test for parsing errors on frontend split queries

Signed-off-by: Mariana Franco <[email protected]>

* Fix lint errors

Signed-off-by: Mariana Franco <[email protected]>

* Merging error test cases with success ones

Signed-off-by: Mariana Franco <[email protected]>
Signed-off-by: Alvin Lin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants