Skip to content

Conversation

wayyoungboy
Copy link
Contributor

Description

Please explain the changes you made here.
When running the TestRowsColumnTypes unit test on OceanBase, I encountered a panic and the test progress was forced to exit.
image

Adding this defensive strategy can prevent the occurrence of panic issues and thus prevent blocking the execution of the unit test.

Checklist

  • Code compiles correctly
  • Created tests which fail without the change (if possible)
  • All tests passing
  • Extended the README / documentation, if necessary
  • Added myself / the copyright holder to the AUTHORS file

@methane
Copy link
Member

methane commented May 4, 2023

It seems just hiding real problem. I don't think it is good idea.

@methane methane closed this May 4, 2023
driver_test.go Outdated
continue
}
}

Copy link
Member

Choose a reason for hiding this comment

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

Put this code here:

if t.Failed() {
    return
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

like this code?
I tested it and the code does seem more appropriate.

		for i := range values {
			if types[i] == nil {
				t.Failed()
				return
			}
			values[i] = reflect.New(types[i]).Interface()
		}

Copy link
Member

Choose a reason for hiding this comment

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

No. Just put my code here.

I think types[i] == nil happens only when test is already failed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agree.
I will resubmit a pull request.

@methane methane reopened this May 4, 2023
driver_test.go Outdated
for i := range values {
if types[i] == nil {
continue
}
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't make sense at all. Remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agree

@wayyoungboy wayyoungboy closed this May 6, 2023
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.

2 participants