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

Fix thin jar caching when testing the optimized executable #43582

Conversation

Thushara-Piyasekara
Copy link
Contributor

Purpose

$title
Fixes #43579

Approach

When we run the bal test --eliminate-dead-code command, we have to duplicate certain modules to avoid false positive test results. During DCE we categorize modules into three categories,

  • Optimized modules :- These are used only by the build project. If there are no test cases, all modules will fall under this category. These are cached in the target/cache directory of the project.
  • Unoptimized modules :- Modules used by the testable pkg. We have to keep these modules pure to avoid false positive test results. See this document for more details. Since these are pure modules, they are cached in the .ballerina file system cache.
  • Optimized duplicated modules:- Common dependencies used by both build project and testable project. These duplicated modules are optimized and used by the build project. They are bytecode instrumented to avoid clashing with their counterparts in testable pkg dependencies. References in build pkg are also bytecode instrumented to make them visible only to build pkg. These are cached in the target/cache directory of the project along with a _OPTIMIZED suffix for the thin jar.

The error was due to a bug in the retrieval of the cache locations during test execution.

Check List

  • Read the Contributing Guide
  • Updated Change Log
  • Checked Tooling Support (#)
  • Added necessary tests
    • Unit Tests
    • Spec Conformance Tests
    • Integration Tests
    • Ballerina By Example Tests
  • Increased Test Coverage
  • Added necessary documentation
    • API documentation
    • Module documentation in Module.md files
    • Ballerina By Examples

@Thushara-Piyasekara
Copy link
Contributor Author

Thushara-Piyasekara commented Nov 19, 2024

@ayeshLK as of now, ballerina/random is passing all the tests. But ballerina/os is giving the following failures,

Running Tests

	os

	testExecExit has failed.


	testExecNegative has failed.


	testExecWithOutputStdErr has failed.


	testExecWithOutputStdOut has failed.


		[fail] testExecExit:

		   error("No such error: ProcessExecError")
				callableName: nativeOutput moduleName: ballerina.os.1 fileName: process.bal lineNumber: 67
				callableName: output moduleName: ballerina.os.1.Process fileName: process.bal lineNumber: 44
				callableName: testExecExit moduleName: ballerina.os$test.1.tests.os_test fileName: tests/os_test.bal lineNumber: 231
				callableName: testExecExit$lambda13$ moduleName: ballerina.os$test.1.tests.test_execute-generated_1 fileName: tests/test_execute-generated_1.bal lineNumber: 17
			

		[fail] testExecNegative:

		   error("No such error: ProcessExecError")
				callableName: exec moduleName: ballerina.os.1 fileName: os.bal lineNumber: 130
				callableName: testExecNegative moduleName: ballerina.os$test.1.tests.os_test fileName: tests/os_test.bal lineNumber: 249
				callableName: testExecNegative$lambda14$ moduleName: ballerina.os$test.1.tests.test_execute-generated_1 fileName: tests/test_execute-generated_1.bal lineNumber: 18
			

		[fail] testExecWithOutputStdErr:

		   error("No such error: ProcessExecError")
				callableName: exec moduleName: ballerina.os.1 fileName: os.bal lineNumber: 130
				callableName: testExecWithOutputStdErr moduleName: ballerina.os$test.1.tests.os_test fileName: tests/os_test.bal lineNumber: 197
				callableName: testExecWithOutputStdErr$lambda11$ moduleName: ballerina.os$test.1.tests.test_execute-generated_1 fileName: tests/test_execute-generated_1.bal lineNumber: 15
			

		[fail] testExecWithOutputStdOut:

		   error("No such error: ProcessExecError")
				callableName: exec moduleName: ballerina.os.1 fileName: os.bal lineNumber: 130
				callableName: testExecWithOutputStdOut moduleName: ballerina.os$test.1.tests.os_test fileName: tests/os_test.bal lineNumber: 182
				callableName: testExecWithOutputStdOut$lambda10$ moduleName: ballerina.os$test.1.tests.test_execute-generated_1 fileName: tests/test_execute-generated_1.bal lineNumber: 14
			


		17 passing
		4 failing
		0 skipped

		Test execution time : 0.075s
error: there are test failures

These seems to be external dependencies used in java side. Following is the ErrorCreator usage of ProccessExecError,

https://github.com/ballerina-platform/module-ballerina-os/blob/3fa45fea3557f009b6e828e6275fd9b2bf12572b/native/src/main/java/io/ballerina/stdlib/os/nativeimpl/WaitForExit.java#L44

Could you please check and confirm?

Copy link
Member

@MaryamZi MaryamZi left a comment

Choose a reason for hiding this comment

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

Let's address the changes separately.

@@ -161,12 +161,20 @@ private void addCodeGeneratedLibraryPaths(PackageContext packageContext, Platfor
}
ModuleContext moduleContext = packageContext.moduleContext(moduleId);
PackageID pkgID = moduleContext.descriptor().moduleCompilationId();
// DuplicatePkgs are pkgs which are imported by both testable and build projects.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// DuplicatePkgs are pkgs which are imported by both testable and build projects.
// Duplicate pkgs are pkgs which are imported by both testable and build projects.

addOptimizedLibraryPaths(packageContext, scope, libraryPaths, moduleContext, pkgID);
addOptimizedLibraryPaths(packageContext, scope, libraryPaths, moduleContext, isDuplicatePkg);
if (!isDuplicatePkg) {
// If the pkg is not an duplicate pkg, we only need the optimized thin JAR.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// If the pkg is not an duplicate pkg, we only need the optimized thin JAR.
// If the pkg is not a duplicate pkg, we only need the optimized thin JAR.

// getPlatformSpecificLibrary gets invoked from TestUtils class and there is no proper way to check whether a
// given module is `optimized`, `unoptimized` or `duplicated optimized`. Maybe we can lift the logic to
// JarResolver somehow.
// TODO Find a cleaner way to handle JarPaths for bal test with dead code elimination.
Copy link
Member

Choose a reason for hiding this comment

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

Let's create an issue for this.

// TODO Find a cleaner way to handle JarPaths for bal test with dead code elimination.
if (Files.exists(jarFilePath)) {
return Optional.of(jarFilePath);
} else if (Files.exists(optimizedJarFilePath)) {
Copy link
Member

Choose a reason for hiding this comment

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

Can be just an if.

@MaryamZi MaryamZi merged commit bfe8fe6 into ballerina-platform:feature-codegen-optimizer Nov 19, 2024
2 checks passed
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.

3 participants