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

Make UUID.compareTo() consistent with the JVM. #3413

Merged
merged 2 commits into from Aug 4, 2023

Conversation

Bensonater
Copy link
Contributor

@Bensonater Bensonater commented Jul 29, 2023

Ports UUID fix from scala-js/scala-js#4884 and matches JVM
Fix #3393: UUID comparison doesn't match JVM

@LeeTibbert
Copy link
Contributor

LeeTibbert commented Jul 31, 2023

@bensonator, Good to see a new contributor. Thank you for this PR.

Review is meant to help a contribution become more useful to end users in
the present and easier to maintain in the future. It can feel like a hundred
kilometer of unnecessary hoops/requests. I mean to be encouraging,
not discouraging.

Overall, well done.

I encourage you to make further contributions. Are you familiar with the Scala Native Discourse
channel? If you ask there and give a quick description of your areas of interest, I am willing
to bet that our fellow SN contributors can make some suggestions from which you can pick
sophomore contributions.

Please consider the following:

  • This PR is focused on an exact type of behavior. I would expect to see a
    corresponding UUIDTest.scala file or a one line explanation of why
    it is not necessary (i.e. exercised extensively in another, named, Test file).
    Having such a test frequently run in Continuous Integration allows the
    project to detect regressions.

    • In this case, the Scala.js PR contained a 'UUIDTest.scala` file which, on a quick
      reading, looks like it could be ported.

      So that this does not feel like an ever expanding impediment, I suggest first
      trying trial port and seeing if all the tests pass.

      If there are too may Scala.jsisms to port in a reasonable time (tens of minutes,
      not weeks), I suggest cutting out all of the tests except the test compareTo of this
      PR.

    • I believe such a UUIDTest.scala should go into the package org.scalanative.testsuite.javalib.util package
      in the existing directory `/unit-tests/shared/src/test/scala/org/scalanative/testsuite/javalib/util. By
      intention, the names are meant to roughly correspond to the Scala.js locations.

      This will allow the same Test to be run from Java and Scala Native, giving evidence that the code of this PR
      is acting as intended.

      • BEWARE the existing ./unit-tests-ext/shared/src/test/scala/org/scalanative/testsuite/javalib/util/UUIDTestEx.scala.
        That file is useful in its own right but a distraction here. I know, because I hate to say, I fell into the distraction.

        The text of that file explains that it tests a Java feature which is not supplied in the Scala Native distribution.

  • Is this PR a candidate for porting back (backporting) to the Scala Native 0.4.n series? I believe yes.
    I am not sure if the "Backport Candidate" label can be added by a contributor in the "Labels" section
    of the GitHub page or is you need to mention it in the PR and a reviewer will do it.

  • Later: 2023-08-01 Now that CI is all green, the "Development" link to the referenced issue
    shows. All set there.

    Historical, Reasons are OK, but github trigger details are wrong:
    The "Resolves issue UUID comparison doesn't match JVM #3393" line is helpful to people trying to trace the impact of this PR both
    now and in the future. GitHub has some magic where it links a PR with the corresponding
    Issue if you get the "Pretty please" just right. I do not see anything under the "Notifications"
    link on the right hand side of the page. This PR is "all green" in CI, so I went looking for it.
    (I think it does not appear until CI passes, but that might just be my superstition.)

    GitHub documents, somewhere the magic words. I usually use "Fix #nnnn" because it
    works for me and I have limited brain space. Here I think the "issue" in "Resolves issue UUID comparison doesn't match JVM #3393"
    may be tripping up GitHub.

    The relevance of this is that getting the link makes life easier for the person who ends up doing
    the merge because the linked Issue gets automatically closed. ~~

  • The PR base note helpfully gives the Scala.js reference. A convention which is quite useful
    to the future maintainers is to add a line at the top of each file such as
    this line from TreeMapTest.scala: // Ported from Scala.js commit def516f dated: 2023-01-22
    You will see different date formats in different files. Something international is courteous to
    the wide range of Scala Native contributors. Something like the dreaded "06/07/2021" causes a future
    maintainer to spend extra cycles when trying to lookup the commit ID.

    Maintainence is almost always done when there is little time and little room for aggravation, so a minute
    or two at the PR end can earn the gratitude of future colleagues.

PS: Part of my normal round when reading a PR is to ask "Are any documentation changes indicated".
Here I concur that no doc changes are necessary. Just want to "Show my work", especially since
the result does not show. Documentation is all to often forgotten by developers and missed by
people who want to use the contribution.

@LeeTibbert
Copy link
Contributor

@Besonator

Looks all green (successful) except for one Windows Scripted test. The failure looks concerning,
but I believe you can ignore it.

Reading CI logs is somewhat of an art. Useful in the abstract, to see what is going on in CI
underneath the covers but arcane.

Here, it looks to me that the Windows test timed out after approximately 6 hours. The SN CI
infrastructure determined that it was unlikely to terminate on its own. The practical result
is that the entire CI will probably be re-run by an/the SN administrator (Wojciech) before
any merge.

We have been trying to determine and eliminate the root cause of CI failures. It is a work of
ages. In this case, looked like a process or test hung, somebody dropped an interrupt or did
an infinite timeout. Chasing intermittent failures, especially in an environment as complex as CI is hard.
Knowing they exist is an early step towards fixing them.

@ekrich ekrich added the backport candidate PR which might be backported into previous major release of SN label Jul 31, 2023
@ekrich
Copy link
Member

ekrich commented Jul 31, 2023

What you have in the description is great. Normally we use the following for PRs that fix an issue using the issue number NNN and the name of the issue.

Fix #NNN: Name of the issue

@WojciechMazur WojciechMazur merged commit f3b1a31 into scala-native:main Aug 4, 2023
79 checks passed
@Bensonater Bensonater deleted the fix-uuid branch August 9, 2023 10:48
WojciechMazur pushed a commit to WojciechMazur/scala-native that referenced this pull request Sep 1, 2023
* Make UUID.compareTo() consistent with the JVM.
* Ported unit tests for UUID

(cherry picked from commit f3b1a31)
WojciechMazur pushed a commit that referenced this pull request Sep 4, 2023
* Make UUID.compareTo() consistent with the JVM.
* Ported unit tests for UUID

(cherry picked from commit f3b1a31)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport candidate PR which might be backported into previous major release of SN component:javalib
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UUID comparison doesn't match JVM
4 participants