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.

Sign up for GitHub

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

Jump to bottom

Fix tests to assert correct error #2546

Merged
merged 18 commits into from
May 17, 2023
Merged

Fix tests to assert correct error #2546

merged 18 commits into from
May 17, 2023

Conversation

chilagrow
Copy link
Member

@chilagrow chilagrow commented May 2, 2023 •
edited
Loading

Description

Closes #2545.
Closes #2345.

Readiness checklist

  • I added/updated unit tests.
  • I added/updated integration/compatibility tests.
  • I added/updated comments and checked rendering.
  • I made spot refactorings.
  • I updated user documentation.
  • I ran task all, and it passed.
  • I ensured that PR title is good enough for the changelog.
  • (for maintainers only) I set Reviewers (@FerretDB/core), Labels, Project and project's Sprint fields.
  • I marked all done items in this checklist.

@chilagrow chilagrow self-assigned this May 2, 2023
@chilagrow chilagrow changed the title command name can be lowercase Fix findAndMofidy error type May 2, 2023
@chilagrow chilagrow added the code/bug Some user-visible feature works incorrectly label May 2, 2023
@chilagrow chilagrow mentioned this pull request May 2, 2023
Closed
@codecov
Copy link

codecov bot commented May 2, 2023 •
edited
Loading

Codecov Report

Merging #2546 (cf9da3c) into main (c067537) will increase coverage by 0.01%.
The diff coverage is 82.35%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2546      +/-   ##
==========================================
+ Coverage   66.19%   66.21%   +0.01%     
==========================================
  Files         429      429              
  Lines       20954    20963       +9     
==========================================
+ Hits        13870    13880      +10     
+ Misses       6150     6148       -2     
- Partials      934      935       +1
Impacted Files Coverage Δ
integration/helpers.go 78.51% <82.35%> (+4.26%) ⬆️

... and 2 files with indirect coverage changes

Flag Coverage Δ
integration 59.16% <82.35%> (-0.03%) ⬇️
mongodb 5.29% <0.00%> (-0.01%) ⬇️
pg 59.07% <82.35%> (-0.03%) ⬇️
unit 26.65% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@chilagrow chilagrow changed the title Fix findAndMofidy error type Fix findAndMofidy error type. May 8, 2023
@chilagrow chilagrow changed the title Fix findAndMofidy error type. Fix findAndMofidy error type May 8, 2023
@chilagrow chilagrow changed the title Fix findAndMofidy error type Fix tests to assert correct error May 11, 2023
@chilagrow chilagrow added code/chore Code maintenance improvements and removed code/bug Some user-visible feature works incorrectly labels May 11, 2023
@chilagrow chilagrow marked this pull request as ready for review May 11, 2023 10:29
@chilagrow chilagrow requested a review from a team as a code owner May 11, 2023 10:29
@chilagrow chilagrow requested review from AlekSi and w84thesun May 11, 2023 10:29
@chilagrow chilagrow enabled auto-merge (squash) May 11, 2023 10:29
@chilagrow chilagrow requested review from a team, rumyantseva and noisersup May 11, 2023 10:29
@AlekSi AlekSi added this to the Next milestone May 11, 2023
w84thesun
w84thesun previously approved these changes May 11, 2023
View reviewed changes
Copy link
Contributor

@w84thesun w84thesun left a comment

Choose a reason for hiding this comment

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

LGTM!

@chilagrow chilagrow changed the title Fix tests to assert correct error Fix tests to assert correct error May 11, 2023
AlekSi
AlekSi requested changes May 11, 2023
View reviewed changes
integration/commands_administration_compat_test.go Outdated Show resolved Hide resolved
integration/helpers.go Outdated
Comment on lines 233 to 234
// driver sent an error which is not WriteException, check errors are the same.
assert.Equal(t, expected, actual)
Copy link
Member

Choose a reason for hiding this comment

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

That would happily allow AssertMatchesWriteError(t, io.EOF, io.EOF), but that contradicts the function description

Copy link
Member Author

Choose a reason for hiding this comment

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

I updated function description.

Copy link
Member Author

Choose a reason for hiding this comment

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

FYI, currently I'm seeing error from the driver that update needs to have at least one field and cannot be empty. I guess we are testing that in few tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

Example https://github.com/FerretDB/FerretDB/actions/runs/4955317748/jobs/8864615428

Copy link
Member

@AlekSi AlekSi May 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

AssertMatchesWriteError should check that both errors are WriteErrors (wrapped in WriteExceptions). It should not consider other error types to be acceptable. That what was wrong in the previous code and what is still wrong now

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated to assert only for WriteError. For ReplaceEmptyDocument test, given the error comes from mongo go driver https://github.com/mongodb/mongo-go-driver/blob/master/mongo/mongo.go#L158, not sure if we want to test driver errors. But for now, I'm checking that specific error message for target and compat before calling WriteError.

FerretDB/integration/update_compat_test.go

Line 435 in bab38ae

"ReplaceEmptyDocument": {

integration/helpers.go Outdated Show resolved Hide resolved
rumyantseva
rumyantseva previously approved these changes May 12, 2023
View reviewed changes
Copy link
Contributor

@rumyantseva rumyantseva left a comment

Choose a reason for hiding this comment

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

LGTM

w84thesun
w84thesun previously approved these changes May 12, 2023
View reviewed changes
noisersup
noisersup previously approved these changes May 12, 2023
View reviewed changes
Copy link
Member

@noisersup noisersup left a comment

Choose a reason for hiding this comment

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

Looks good to me! Are we planning to introduce comparison of messages in the future? To stay independent from data we can use regexes or replace the values with placeholders before comparison

@rumyantseva
Copy link
Contributor

@noisersup we have this issue: #2599 :)

@chilagrow chilagrow mentioned this pull request May 15, 2023
Closed
@chilagrow chilagrow dismissed stale reviews from noisersup, w84thesun, and rumyantseva via 34e85be May 15, 2023 00:28
w84thesun
w84thesun previously approved these changes May 15, 2023
View reviewed changes
Copy link
Contributor

@w84thesun w84thesun left a comment

Choose a reason for hiding this comment

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

LGTM! Nice catch with multi flag!

rumyantseva
rumyantseva previously approved these changes May 15, 2023
View reviewed changes
Copy link
Contributor

@rumyantseva rumyantseva left a comment

Choose a reason for hiding this comment

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

LGTM. The checks look reasonable!

I'm not sure about the naming (left a suggestion about naming), but I don't really like my suggestion as the functions have different signatures (different error types).

integration/helpers.go Show resolved Hide resolved
noisersup
noisersup previously approved these changes May 15, 2023
View reviewed changes
@noisersup noisersup assigned AlekSi and unassigned chilagrow May 17, 2023
@AlekSi AlekSi dismissed stale reviews from noisersup, rumyantseva, and w84thesun via 7836630 May 17, 2023 10:24
@AlekSi AlekSi disabled auto-merge May 17, 2023 20:51
@AlekSi AlekSi merged commit 2716382 into FerretDB:main May 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@noisersup noisersup noisersup left review comments

@rumyantseva rumyantseva rumyantseva left review comments

@w84thesun w84thesun w84thesun left review comments

@AlekSi AlekSi Awaiting requested review from AlekSi

Assignees

@AlekSi AlekSi

Labels
code/chore Code maintenance improvements
Projects
Current Engineering
Archived in project
Milestone
v1.2.0
Development

Successfully merging this pull request may close these issues.

Fix test helpers and tests to assert correct error Do not check error messages in compatibility tests
5 participants
@chilagrow @rumyantseva @AlekSi @w84thesun @noisersup