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.

Sign up for GitHub

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

Jump to bottom

Run whole test/Base_Tests with NI #10296

Merged
merged 58 commits into from
Jun 21, 2024
Merged

Run whole test/Base_Tests with NI #10296

merged 58 commits into from
Jun 21, 2024

Conversation

JaroslavTulach
Copy link
Member

@JaroslavTulach JaroslavTulach commented Jun 17, 2024 •
edited
Loading

Pull Request Description

Fixes #10126 by various changes to make more test/Base_Tests pass in the Enso mode.

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • All code follows the
    Java,
    Rust
    style guides.
  • Test whole test/Base_Tests via ./runner in CI

@JaroslavTulach JaroslavTulach added the CI: No changelog needed Do not require a changelog entry for this PR. label Jun 17, 2024
@JaroslavTulach JaroslavTulach self-assigned this Jun 17, 2024
@JaroslavTulach
Copy link
Member Author

JaroslavTulach commented Jun 17, 2024 •
edited
Loading

We can run whole ./runner --run test/Base_Tests and not crash now. That's marks the first milestone:

after 20f9589 fixes we are down to

after d61e5d2 we have

finally (for today) with 501c07d we are down to

@JaroslavTulach JaroslavTulach force-pushed the wip/jtulach/AllBaseTestsInNi10126 branch from 0248742 to a1bff6e Compare June 20, 2024 11:30
@JaroslavTulach JaroslavTulach mentioned this pull request Jun 20, 2024
Open
Akirathan
Akirathan approved these changes Jun 20, 2024
View reviewed changes
Copy link
Member

@Akirathan Akirathan left a comment

Choose a reason for hiding this comment

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

This is a huge step forward. Congratulations on making all the tests pass in such a short time. Will there be a follow-up PR enabling test/Table_Tests as well?

distribution/lib/Standard/Base/0.0.0-dev/src/Data/Time/Date_Period.enso
@@ -11,7 +11,10 @@ polyglot java import java.time.temporal.TemporalAdjuster
polyglot java import java.time.temporal.TemporalAdjusters
polyglot java import java.time.temporal.TemporalUnit
polyglot java import org.enso.base.time.CustomTemporalUnits
polyglot java import org.enso.base.time.Date_Utils
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't these imports also be kept in Extra_Imports.enso?

Copy link
Member Author

Choose a reason for hiding this comment

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

My decision making process was:

  • if the failure is in *_Spec.enso file, put the polyglot java import into Extra_Imports.enso
  • if the failure is in Standard.Base library file, put the import into that file.

In this case the failure was in Date_Period.enso file, I believe.

build/build/src/engine/context.rs Show resolved Hide resolved
@JaroslavTulach
Copy link
Member Author

This is a huge step forward. Congratulations on making all the tests pass in such a short time. Will there be a follow-up PR enabling test/Table_Tests as well?

The next step is to implement

-e.g. work on support for --jvm option, detection of compiled & 3rd party libraries as described in the #10121 discussion. From a perspective of that task, it is important to have some "3rd party library" (not compiled by NI). Standard.Table can play such a role.

@JaroslavTulach JaroslavTulach merged commit fe2cf49 into develop Jun 21, 2024
41 checks passed
@JaroslavTulach JaroslavTulach deleted the wip/jtulach/AllBaseTestsInNi10126 branch June 21, 2024 04:03
@JaroslavTulach JaroslavTulach mentioned this pull request Jun 26, 2024
Merged
5 tasks
This was referenced Jul 10, 2024
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GregoryTravis GregoryTravis GregoryTravis approved these changes

@Akirathan Akirathan Akirathan approved these changes

@kazcw kazcw kazcw approved these changes

@hubertp hubertp hubertp approved these changes

@4e6 4e6 Awaiting requested review from 4e6 4e6 is a code owner

@jdunkerley jdunkerley Awaiting requested review from jdunkerley jdunkerley is a code owner

@AdRiley AdRiley Awaiting requested review from AdRiley AdRiley is a code owner

@Frizi Frizi Awaiting requested review from Frizi Frizi is a code owner

@radeusgd radeusgd Awaiting requested review from radeusgd radeusgd is a code owner

@marthasharkey marthasharkey Awaiting requested review from marthasharkey marthasharkey is a code owner

@farmaazon farmaazon Awaiting requested review from farmaazon farmaazon is a code owner

@vitvakatu vitvakatu Awaiting requested review from vitvakatu vitvakatu is a code owner

Assignees

@JaroslavTulach JaroslavTulach

Labels
CI: Clean build required CI runners will be cleaned before and after this PR is built. CI: No changelog needed Do not require a changelog entry for this PR.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Run whole Standard.Base in Native Image ./runner
6 participants
@JaroslavTulach @hubertp @kazcw @radeusgd @Akirathan @GregoryTravis