Treat all test package imports as test imports (sorbet#9595)

elliottt · web-flow · commit bf70f5c170ca · 2025-11-07T15:31:57.000-08:00
* Add a test

* Treat all test package imports as test imports

* Update tests
diff --git a/packager/ComputePackageSCCs.h b/packager/ComputePackageSCCs.h
@@ -138,22 +138,26 @@ class ComputePackageSCCs {
             // we've visited all members of the SCC once, to ensure that their ids have all been populated.
             for (auto name : condensationNode.members) {
                 for (auto &i : packageGraph.getImports(name)) {
-                    // We want to consider all imports from test code, but only normal imports for application code.
+                    // The mangled name won't exist if the import was to a package that doesn't exist.
+                    if (!i.mangledName.exists()) {
+                        continue;
+                    }
+
+                    int impId = 0;
+
                     if constexpr (EdgeType == core::packages::ImportType::Normal) {
                         if (i.type != core::packages::ImportType::Normal) {
                             continue;
                         }
+                        impId = packageGraph.getSCCId(i.mangledName);
+                    } else {
+                        // If we're processing imports from a test package, we can't tell if those imports are used only
+                        // at application time, or application and test time. Conservatively, we consider the import to
+                        // be to the test package, which transitively depends on the application code, ensuring the
+                        // correct ordering for test packages.
+                        impId = packageGraph.getTestSCCId(i.mangledName);
                     }
 
-                    // The mangled name won't exist if the import was to a package that doesn't exist.
-                    if (!i.mangledName.exists()) {
-                        continue;
-                    }
-
-                    // All of the imports of every member of the SCC will have been processed in the recursive step, so
-                    // we can assume the scc id of the target exists. Additionally, all imports are to the original
-                    // application code, which is why we don't consider using the `testSccID` here.
-                    auto impId = packageGraph.getSCCId(i.mangledName);
                     if (impId == sccId) {
                         continue;
                     }
diff --git a/packager/packager.cc b/packager/packager.cc
@@ -1210,6 +1210,12 @@ class PackageDBPackageGraph {
         return packageDB.getPackageInfo(packageName).sccID().value();
     }
 
+    int getTestSCCId(MangledName packageName) const {
+        ENFORCE(packageDB.getPackageInfo(packageName).exists());
+        ENFORCE(packageDB.getPackageInfo(packageName).testSccID().has_value());
+        return packageDB.getPackageInfo(packageName).testSccID().value();
+    }
+
     void setTestSCCId(MangledName packageName, int sccID) {
         auto *pkgInfoPtr = packageDB.getPackageInfoNonConst(packageName);
         if (!pkgInfoPtr) {
diff --git a/test/cli/condensation-package-autocorrect-missing-import/test.out b/test/cli/condensation-package-autocorrect-missing-import/test.out
@@ -335,16 +335,16 @@ use_cycle_package/foo.rb:13: Unable to resolve constant `Test` https://srb.help/
     NN |module Net
         ^^^^^^^^^^
 
-use_cycle_package/foo.test.rb:4: `Test::Foo::Bar::CyclePackage::TestUtil` resolves but its package is not imported https://srb.help/3718
+use_cycle_package/foo.test.rb:4: Unable to resolve constant `Bar` https://srb.help/5002
      4 |  Test::Foo::Bar::CyclePackage::TestUtil
-          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-    cycle_package/__package.rb:5: Exported from package here
-     5 |class Foo::Bar::CyclePackage < PackageSpec
-        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+          ^^^^^^^^^^^^^^
   Autocorrect: Done
-    use_cycle_package/__package.rb:7: Inserted `test_import Foo::Bar::CyclePackage, only: "test_rb"`
-     7 |  strict_dependencies 'layered_dag'
-                                           ^
+    use_cycle_package/foo.test.rb:4: Replaced with `Dir`
+     4 |  Test::Foo::Bar::CyclePackage::TestUtil
+          ^^^^^^^^^^^^^^
+    https://github.com/sorbet/sorbet/tree/master/rbi/core/dir.rbi#LCENSORED: `Dir` defined here
+    NN |class Dir < Object
+        ^^^^^^^^^^^^^^^^^^
 Errors: 4
 # frozen_string_literal: true
 # typed: strict
@@ -353,7 +353,6 @@ Errors: 4
 class Foo::MyPackage < PackageSpec
   layer 'lib'
   strict_dependencies 'layered_dag'
-  test_import Foo::Bar::CyclePackage, only: "test_rb"
   test_import Foo::Bar::CyclePackageTest
 end
 
@@ -399,16 +398,16 @@ use_app_cycle_package/foo.rb:13: Unable to resolve constant `Test` https://srb.h
     NN |module Net
         ^^^^^^^^^^
 
-use_app_cycle_package/foo.test.rb:4: `Test::Foo::Bar::AppCyclePackage::TestUtil` resolves but its package is not imported https://srb.help/3718
+use_app_cycle_package/foo.test.rb:4: Unable to resolve constant `Bar` https://srb.help/5002
      4 |  Test::Foo::Bar::AppCyclePackage::TestUtil
-          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-    app_cycle_package/__package.rb:5: Exported from package here
-     5 |class Foo::Bar::AppCyclePackage < PackageSpec
-        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-  Autocorrect: Done
-    use_app_cycle_package/__package.rb:7: Inserted `test_import Foo::Bar::AppCyclePackage, only: "test_rb"`
-     7 |  strict_dependencies 'layered_dag'
-                                           ^
+          ^^^^^^^^^^^^^^
+  Autocorrect: Done
+    use_app_cycle_package/foo.test.rb:4: Replaced with `Dir`
+     4 |  Test::Foo::Bar::AppCyclePackage::TestUtil
+          ^^^^^^^^^^^^^^
+    https://github.com/sorbet/sorbet/tree/master/rbi/core/dir.rbi#LCENSORED: `Dir` defined here
+    NN |class Dir < Object
+        ^^^^^^^^^^^^^^^^^^
 Errors: 4
 # frozen_string_literal: true
 # typed: strict
@@ -417,7 +416,6 @@ Errors: 4
 class Foo::MyPackage < PackageSpec
   layer 'lib'
   strict_dependencies 'layered_dag'
-  test_import Foo::Bar::AppCyclePackage, only: "test_rb"
   test_import Foo::Bar::AppCyclePackageTest
 end
 
@@ -463,16 +461,16 @@ use_false_cycle_package/foo.rb:13: Unable to resolve constant `Test` https://srb
     NN |module Net
         ^^^^^^^^^^
 
-use_false_cycle_package/foo.test.rb:4: `Test::Foo::Bar::FalseCyclePackage::TestUtil` resolves but its package is not imported https://srb.help/3718
+use_false_cycle_package/foo.test.rb:4: Unable to resolve constant `Bar` https://srb.help/5002
      4 |  Test::Foo::Bar::FalseCyclePackage::TestUtil
-          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-    false_cycle_package/__package.rb:5: Exported from package here
-     5 |class Foo::Bar::FalseCyclePackage < PackageSpec
-        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-  Autocorrect: Done
-    use_false_cycle_package/__package.rb:7: Inserted `test_import Foo::Bar::FalseCyclePackage, only: "test_rb"`
-     7 |  strict_dependencies 'layered_dag'
-                                           ^
+          ^^^^^^^^^^^^^^
+  Autocorrect: Done
+    use_false_cycle_package/foo.test.rb:4: Replaced with `Dir`
+     4 |  Test::Foo::Bar::FalseCyclePackage::TestUtil
+          ^^^^^^^^^^^^^^
+    https://github.com/sorbet/sorbet/tree/master/rbi/core/dir.rbi#LCENSORED: `Dir` defined here
+    NN |class Dir < Object
+        ^^^^^^^^^^^^^^^^^^
 Errors: 4
 # frozen_string_literal: true
 # typed: strict
@@ -481,7 +479,6 @@ Errors: 4
 class Foo::MyPackage < PackageSpec
   layer 'lib'
   strict_dependencies 'layered_dag'
-  test_import Foo::Bar::FalseCyclePackage, only: "test_rb"
   test_import Foo::Bar::FalseCyclePackageTest
 end
 
@@ -527,16 +524,16 @@ use_app_false_cycle_package/foo.rb:13: Unable to resolve constant `Test` https:/
     NN |module Net
         ^^^^^^^^^^
 
-use_app_false_cycle_package/foo.test.rb:4: `Test::Foo::Bar::AppFalseCyclePackage::TestUtil` resolves but its package is not imported https://srb.help/3718
+use_app_false_cycle_package/foo.test.rb:4: Unable to resolve constant `Bar` https://srb.help/5002
      4 |  Test::Foo::Bar::AppFalseCyclePackage::TestUtil
-          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-    app_false_cycle_package/__package.rb:5: Exported from package here
-     5 |class Foo::Bar::AppFalseCyclePackage < PackageSpec
-        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-  Autocorrect: Done
-    use_app_false_cycle_package/__package.rb:7: Inserted `test_import Foo::Bar::AppFalseCyclePackage, only: "test_rb"`
-     7 |  strict_dependencies 'layered_dag'
-                                           ^
+          ^^^^^^^^^^^^^^
+  Autocorrect: Done
+    use_app_false_cycle_package/foo.test.rb:4: Replaced with `Dir`
+     4 |  Test::Foo::Bar::AppFalseCyclePackage::TestUtil
+          ^^^^^^^^^^^^^^
+    https://github.com/sorbet/sorbet/tree/master/rbi/core/dir.rbi#LCENSORED: `Dir` defined here
+    NN |class Dir < Object
+        ^^^^^^^^^^^^^^^^^^
 Errors: 4
 # frozen_string_literal: true
 # typed: strict
@@ -545,6 +542,5 @@ Errors: 4
 class Foo::MyPackage < PackageSpec
   layer 'lib'
   strict_dependencies 'layered_dag'
-  test_import Foo::Bar::AppFalseCyclePackage, only: "test_rb"
   test_import Foo::Bar::AppFalseCyclePackageTest
 end
diff --git a/test/cli/condensation-package-test-import/a/__package.rb b/test/cli/condensation-package-test-import/a/__package.rb
@@ -0,0 +1,5 @@
+# typed: strict
+
+class A < PackageSpec
+  test_import B
+end
diff --git a/test/cli/condensation-package-test-import/a/test/a.test.rb b/test/cli/condensation-package-test-import/a/test/a.test.rb
@@ -0,0 +1,6 @@
+# typed: strict
+
+module Test::A
+  class Example < Test::B::Helper
+  end
+end
diff --git a/test/cli/condensation-package-test-import/b/__package.rb b/test/cli/condensation-package-test-import/b/__package.rb
@@ -0,0 +1,6 @@
+# typed: strict
+
+class B < PackageSpec
+  test_import C
+  export Test::B::Helper
+end
diff --git a/test/cli/condensation-package-test-import/b/test/helper.rb b/test/cli/condensation-package-test-import/b/test/helper.rb
@@ -0,0 +1,6 @@
+# typed: strict
+
+module Test::B
+  class Helper
+  end
+end
diff --git a/test/cli/condensation-package-test-import/c/__package.rb b/test/cli/condensation-package-test-import/c/__package.rb
@@ -0,0 +1,5 @@
+# typed: strict
+
+class C < PackageSpec
+  import B
+end
diff --git a/test/cli/condensation-package-test-import/test.out b/test/cli/condensation-package-test-import/test.out
@@ -0,0 +1 @@
+No errors! Great job.
diff --git a/test/cli/condensation-package-test-import/test.sh b/test/cli/condensation-package-test-import/test.sh
@@ -0,0 +1,15 @@
+#!/usr/bin/env bash
+
+# This test exercises test helper dependencies in the condensation graph,
+# ensuring that we model those dependencies correctly. In the example, the test
+# package for A must be checked after the test package for B, with C existing to
+# push the B test package later in the condensation graph traversal. If the
+# test_import dependency isn't correctly modeled in the condensation graph, A's
+# test package might be checked before B's, at which point we'll see a
+# resolution error for the superclass in A's test.
+
+cd test/cli/condensation-package-test-import || exit 1
+
+../../../main/sorbet --silence-dev-message --stripe-packages \
+  --experimental-package-directed \
+  --max-threads=0 a b c 2>&1
