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

Port to JUnit java.lang #1955

Merged
merged 23 commits into from Oct 20, 2020
Merged

Port to JUnit java.lang #1955

merged 23 commits into from Oct 20, 2020

Conversation

ekrich
Copy link
Member

@ekrich ekrich commented Oct 19, 2020

There are some small naming updates to ThreadTest which was already ported.

Here is a unified diff of the rest. https://gist.github.com/ekrich/bcde340a67bca52d4da90f64d296579a

@ekrich
Copy link
Member Author

ekrich commented Oct 19, 2020

@sjrd Sorry about that, I needed to format a couple of files.

Copy link
Contributor

@LeeTibbert LeeTibbert left a comment

Choose a reason for hiding this comment

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

Eric, thank you for this huge body of work. LGTM.

I studied the ProcessTest changes in detail, because of some work to be done after the JUnit changes settle. The intention, as I understand it, was to do a one-for-one transform. I think you
were successful at that. The bugs I was chasing, and you had no intent of fixing,
still exist for me to fix and I discovered a new set whilst reviewing.

I did a spot check of other files, and as expected, all was well. I did notice what I think will
be a major improvement. Across the board, the Suites used assert(), often times quite a few in one test, with no way to tell which one failed. I believe that JUnit assertEquals will at least print out the expected and the unexpected values, making it easier to figure out which assertion might be failing. I believe that asserTrue() does not give us this information for free.

Someday, every assertion will be uniquely identifiable; either by being the only one of its kind in a test, or by having an explicit message argument (even if only "a1", "a2", etc.)

@ekrich
Copy link
Member Author

ekrich commented Oct 20, 2020

Yes, @sjrd also mentioned about the assertEquals. I transformed straight assert into the JUnit assertTrue so that the JUnit import would be not be unused. This also creates work because the comment field is first in JUnit so I had to swap order. Either way, switching to assertEquals would not be hard now.

As we normalize the shared code between Scala Native and Scala.js we can keep the common code at the top and the Scala Native specific/issue tests at the bottom and Scala.js could do the same. I plan to go and work on toggle case test names in Scala.js after this so if we copy tests the names will be more standard. Anyway, it greatly increases the likelihood of sharing.

@sjrd sjrd merged commit 170c694 into scala-native:master Oct 20, 2020
@ekrich ekrich deleted the topic/junit-lang branch October 20, 2020 15:12
@LeeTibbert
Copy link
Contributor

What I was trying to express is that the necessary one-to-one from assert(e == g) to AssertEquals(e, g) you already did for this PR
gave us some much needed disambiguation for free; that is, for only the labor already spent. A hidden & bonus bang for the
buck/CHF for all the time, drive, & effort.

vicopem pushed a commit to vicopem/scala-native that referenced this pull request Nov 19, 2020
* Port ThrowablesTest

* Update some names to togglecase in ThreadTest

* Port SystemTest

* Port StringTest

* Port StringBuilderTest

* Port StringBufferTest

* Port StackTraceElementTest

* Port ScalaNumberTest

* Port RuntimeTest

* Change asserts

* Remove unused import

* Add ProcessUtils for Runtime and Process tests

* Use ProcessUtils for RuntimeTest

* Port ProcessTest

* Port MathTest

* Port LongTest

* Port IntegerTest

* Port ExceptionTest

* Port ClassTest

* Port FloatTest

* Port DoubleTest

* Port CharacterTest

* Run scalafmt
vicopem added a commit to vicopem/scala-native that referenced this pull request Nov 19, 2020
vicopem added a commit to vicopem/scala-native that referenced this pull request Nov 19, 2020
ekrich added a commit to ekrich/scala-native that referenced this pull request May 21, 2021
* Port ThrowablesTest

* Update some names to togglecase in ThreadTest

* Port SystemTest

* Port StringTest

* Port StringBuilderTest

* Port StringBufferTest

* Port StackTraceElementTest

* Port ScalaNumberTest

* Port RuntimeTest

* Change asserts

* Remove unused import

* Add ProcessUtils for Runtime and Process tests

* Use ProcessUtils for RuntimeTest

* Port ProcessTest

* Port MathTest

* Port LongTest

* Port IntegerTest

* Port ExceptionTest

* Port ClassTest

* Port FloatTest

* Port DoubleTest

* Port CharacterTest

* Run scalafmt
WojciechMazur pushed a commit to WojciechMazur/scala-native that referenced this pull request Aug 25, 2021
* Port ThrowablesTest

* Update some names to togglecase in ThreadTest

* Port SystemTest

* Port StringTest

* Port StringBuilderTest

* Port StringBufferTest

* Port StackTraceElementTest

* Port ScalaNumberTest

* Port RuntimeTest

* Change asserts

* Remove unused import

* Add ProcessUtils for Runtime and Process tests

* Use ProcessUtils for RuntimeTest

* Port ProcessTest

* Port MathTest

* Port LongTest

* Port IntegerTest

* Port ExceptionTest

* Port ClassTest

* Port FloatTest

* Port DoubleTest

* Port CharacterTest

* Run scalafmt
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.

None yet

3 participants