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

Move tests to src/test/scala-2 #4607

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

adkian-sifive
Copy link
Contributor

We're working on getting tests running with Scala3 with a piecemeal approach, so as features get implemented or the tests get fixed we'll move things back to src/main/scala.

Contributor Checklist

  • Did you add Scaladoc to every public function/method?
  • Did you add at least one test demonstrating the PR?
  • Did you delete any extraneous printlns/debugging code?
  • Did you specify the type of improvement?
  • Did you add appropriate documentation in docs/src?
  • Did you request a desired merge strategy?
  • Did you add text to be included in the Release Notes for this change?

Type of Improvement

  • Internal or build-related (includes code refactoring/cleanup)

Desired Merge Strategy

  • Squash: The PR will be squashed and merged (choose this if you have no preference).

Release Notes

Tests moved to scala-2 directory to facilitate smoother Scala 3 development

Reviewer Checklist (only modified by reviewer)

  • Did you add the appropriate labels? (Select the most appropriate one based on the "Type of Improvement")
  • Did you mark the proper milestone (Bug fix: 3.6.x, 5.x, or 6.x depending on impact, API modification or big change: 7.0)?
  • Did you review?
  • Did you check whether all relevant Contributor checkboxes have been checked?
  • Did you do one of the following when ready to merge:
    • Squash: You/ the contributor Enable auto-merge (squash), clean up the commit message, and label with Please Merge.
    • Merge: Ensure that contributor has cleaned up their commit history, then merge with Create a merge commit.

@adkian-sifive adkian-sifive added the Internal Internal change, does not affect users, will be included in release notes label Jan 9, 2025
@adkian-sifive adkian-sifive force-pushed the adkian-sifive/sbt-cross-tests branch from 491627c to 7834b6b Compare January 9, 2025 21:11
@jackkoenig
Copy link
Contributor

Several tests fail:

  circtTests.stage.ChiselStageSpec ChiselStage exception handling should include source line and a caret for recoverable errors
  circtTests.stage.ChiselStageSpec ChiselStage exception handling should NOT include source line and caret with an incorrect --source-root
  chiselTests.SourceLocatorSpec (0) Relative source paths (0.a): are emitted by default relative to `user-dir`
  chiselTests.SourceLocatorSpec (2) Module source locators (2.a): modules extending RawModule should have a source locator
  chiselTests.SourceLocatorSpec (2) Module source locators (2.b): modules extending Module should have a source locator
  chiselTests.SourceLocatorSpec (2) Module source locators (2.c): modules extending other user modules should have a source locator
  chiselTests.SourceLocatorSpec (2) Module source locators (2.d): modules extending BlackBox should have a source locator
  chiselTests.SourceLocatorSpec (2) Module source locators (2.e): modules extending ExtModule should have a source locator
  chiselTests.SourceLocatorSpec (2) Module source locators (2.f): user-defined Classes should have a source locator
  chiselTests.SourceLocatorSpec (2) Module source locators (2.g): Inner and anonymous modules should have a source locators
  chiselTests.SourceLocatorSpec (2) Module source locators (2.h): Definitions should have a source locator
  chiselTests.SourceLocatorSpec (3) SourceLocator.makeMessage() (3.a) Should have click-to-source functionality
  chiselTests.stage.WarningConfigurationSpec Warning Configuration should support source filter globs
  chiselTests.simulator.SimulatorSpec Chisel Simulator reports failed expects correctly
  chiselTests.TypeAliasSpec Duplicate bundle type aliases with differing structures should error
  chiselTests.properties.PropertySpec PropertyArithmeticOps should not support expressions in Classes, and give a nice error
  chiselTests.properties.PropertySpec PropertySeqOps should not support expressions in Classes, and give a nice error
  chiselTests.PortSpec Ports now have source locators

Those all probably have hardcoded source locator paths. You can make the tests a little more flexible, but for at least a couple of tests in circtTests.stage.ChiselStageSpec and chiselTests.SourceLocatorSpec we want to make sure we check the full path--it's important that we don't regress here naively updating to Mill 0.12.0 would break this so we need to ensure it's being checked.

@seldridge
Copy link
Member

You can make the tests a little more flexible

This may be a good candidate for FileCheck?

@adkian-sifive adkian-sifive force-pushed the adkian-sifive/sbt-cross-tests branch from 7834b6b to ce8c00e Compare January 15, 2025 13:03
@adkian-sifive adkian-sifive added the Merge with merge commit Please merge with a merge commit, do not squash and merg label Jan 15, 2025
@jackkoenig
Copy link
Contributor

@adkian-sifive Same (or perhaps a subset) of tests are still failing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Internal Internal change, does not affect users, will be included in release notes Merge with merge commit Please merge with a merge commit, do not squash and merg
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants