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

Renaming Tests Discussion #1505

Open
hntd187 opened this issue Dec 29, 2021 · 8 comments
Open

Renaming Tests Discussion #1505

hntd187 opened this issue Dec 29, 2021 · 8 comments
Labels
enhancement New feature or request

Comments

@hntd187
Copy link
Contributor

hntd187 commented Dec 29, 2021

Is your feature request related to a problem or challenge? Please describe what you are trying to do.

I am starting the discussion on how we want to rename some of the tests post the refactoring of the tests in #1491

Describe the solution you'd like

Some of the tests have names that don't necessarily have anything to do with what they are testing. Most universally the tests that begin with csv_* come to mind. Does that prefix have any value? I think we should just universally remove them. Maybe another suggestion maybe to instead prefix tests with what functionality they are meant to actually test?

Describe alternatives you've considered

Not renaming them?

Additional context
Add any other context or screenshots about the feature request here.

@hntd187 hntd187 added the enhancement New feature or request label Dec 29, 2021
@hntd187
Copy link
Contributor Author

hntd187 commented Jan 4, 2022

@alamb @xudong963 @liukun4515 Any opinion on this or would you prefer I just run with a PR on my own ideas?

@alamb
Copy link
Contributor

alamb commented Jan 4, 2022

Some of the tests have names that don't necessarily have anything to do with what they are testing. Most universally the tests that begin with csv_* come to mind. Does that prefix have any value? I think we should just universally remove them.

Makes sense to me

Maybe another suggestion maybe to instead prefix tests with what functionality they are meant to actually test?

That would also be a great idea

Sorry for the lack of feedback earlier - I can't keep up with all the tickets 🤯 somedays :)

@liukun4515
Copy link
Contributor

@alamb @xudong963 @liukun4515 Any opinion on this or would you prefer I just run with a PR on my own ideas?

@hntd187 agree with you.
Some tests have the meaningless prefix, and it is not necessary.
My suggestion is that the prefix should be aligned with intend of the test.

I can start this task. If you finish it, I would like to review it.

@xudong963
Copy link
Member

Make sense to me, looking forward to your ticket! I can help review it. @hntd187

@liukun4515
Copy link
Contributor

@hntd187
any update for this?

@alamb
Copy link
Contributor

alamb commented Jan 25, 2022

I think this is completed in #1491

@alamb alamb closed this as completed Jan 25, 2022
@hntd187
Copy link
Contributor Author

hntd187 commented Jan 26, 2022

No @alamb this is renaming which we didn’t do in #1491 we agreed I’d do it after some discussion in this issue. I planned to get to this this weekend.

@alamb alamb reopened this Jan 26, 2022
@alamb
Copy link
Contributor

alamb commented Jan 26, 2022

FYI I plan to move some tests out of context.rs into the sql_integration test -- I'll try and get that done prior to the big rename 🏃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants