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

Unifies test naming standard #1975

Merged
merged 5 commits into from
Oct 19, 2017

Conversation

AlejandroME
Copy link
Contributor

@AlejandroME AlejandroME commented Oct 16, 2017

Fixes #1965.

@kailuowang There are other tests in packages like cats.jvm.tests and cats.free that wasn't renamed. Should I have to rename them also?

- Renames tests in `cats.tests` package with the "Suite" suffix,
according to typelevel#1960 and typelevel#1965.
@kailuowang
Copy link
Contributor

kailuowang commented Oct 16, 2017

Thanks a lot!
With these renames, import renames like
import cats.kernel.laws.discipline.{MonoidTests => MonoidLawTests}
will no longer be needed. Will you kindly update them as well?

@codecov-io
Copy link

codecov-io commented Oct 16, 2017

Codecov Report

Merging #1975 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1975   +/-   ##
=======================================
  Coverage   96.21%   96.21%           
=======================================
  Files         272      272           
  Lines        4628     4628           
  Branches      124      124           
=======================================
  Hits         4453     4453           
  Misses        175      175

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0a162dd...6a65551. Read the comment docs.

@AlejandroME
Copy link
Contributor Author

@kailuowang Only for clarifying, do you mean:

  1. Rename tests in cats.jvm.tests and cats.free as well.
  2. Remove the import aliases and use the test name normally

If so, I'll do it ending the day :)

Please let me know

@kailuowang
Copy link
Contributor

@AlejandroME yeah, sorry I wasn't clear. Thanks!

@AlejandroME
Copy link
Contributor Author

@kailuowang I'm almost done, but I have a question. The import aliases that you've mentioned (import cats.kernel.laws.discipline.{MonoidTests => MonoidLawTests}) are pointing to a package that is not contained in test, but all the classes are named with the prefix Test (like the aforementioned one). Should I make the change (rename to XXXXSuite) in that package also? (cats.kernel.laws.discipline).

I ask this question because I'm not sure about the nature of these classes because I don't see them as tests, but classes that help laws checking.

Sorry if it is a newbie question, but I prefer to ask before doing something dangerous.

Thanks!

@kailuowang
Copy link
Contributor

@AlejandroME no it's totally my fault, I was completely blind over the fact that you are new to this part of the code base.

Before your PR, there are two XXXXTests for many XXXXs, one in cats.test and one in cats.laws or cats.kernel.laws. The import aliases were to avoid the conflicts between them. Now that you renamed the ones in cats.tests, we no longer need those import aliases. For example, if a file in cats.tests has

import cats.kernel.laws.discipline.{MonoidTests => MonoidLawTests}

and later on referred to MonoidLawTests, it was because there was a MonoidTests in cats.tests as well.

Now with your changes, we can remove the import alias and just use MonoidTests in the file.
Would this make sense?
Sorry again about the confusion.

@AlejandroME
Copy link
Contributor Author

@kailuowang No worries, and thank you for the clarification!
Sure, hope to finish this today!

@kailuowang
Copy link
Contributor

oh sorry, I forgot to mention that we should rename tests in cats.jvm.tests and cats.free as well.

@AlejandroME
Copy link
Contributor Author

@kailuowang No worries, I'm resolving the conflicting files and then I proceed to rename those files :)

- The previous commit resolved some conflicting files. Also, test
classes in `cats.jvm` and `cats.free` packages were renamed to use
the `XXXXSuite` naming convention.
- Deleted some import aliases to `cats.kernel.laws.discipline` classes
@AlejandroME
Copy link
Contributor Author

@kailuowang I think that it's done. The tests are running ok on my machine.
Waiting for the Travis result and your review :)

@kailuowang kailuowang merged commit b66b121 into typelevel:master Oct 19, 2017
@kailuowang kailuowang added this to the 1.0.0-RC1 milestone Oct 19, 2017
@AlejandroME AlejandroME deleted the testSuiteRenaming branch October 19, 2017 18:09
kailuowang pushed a commit to kailuowang/cats that referenced this pull request Oct 23, 2017
* Unifies tests naming standard

- Renames tests in `cats.tests` package with the "Suite" suffix,
according to typelevel#1960 and typelevel#1965.

* Removes laws import aliases

* Deletes more import alias

- The previous commit resolved some conflicting files. Also, test
classes in `cats.jvm` and `cats.free` packages were renamed to use
the `XXXXSuite` naming convention.
- Deleted some import aliases to `cats.kernel.laws.discipline` classes
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.

5 participants