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

Implement bal test --cloud for executing tests in docker containers #42183

Merged
merged 16 commits into from
May 10, 2024

Conversation

Ramith-D-Rodrigo
Copy link
Contributor

@Ramith-D-Rodrigo Ramith-D-Rodrigo commented Feb 19, 2024

Purpose

To execute ballerina project tests in docker container in order to verify the business logic functionality within these said containers. To this end, a new flag named "--cloud" for bal test command is required.

Fixes #41036

Approach

Creates a standalone fat jar that contains all the test modules and the relevant test execution dependencies.
Once created, the C2C lifecycle plugin will notify the C2C for generating the docker artifacts. Since the test execution is done at C2C side, the RunTestsTask will not be executed. Hence, features such as code coverage and test reports are not supported.

Currently, implemented only for the JVM path.

Unit tests will be made after the initial code review.

Samples

Provide high-level details about the samples related to this feature.

Remarks

TODO: implement the GraalVM path execution.

After 4084925 commit (including the previous commits that were done recently), GraalVM path execution works for the cloud 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


import io.ballerina.projects.util.ProjectConstants;

import java.io.*;
Copy link
Contributor

Choose a reason for hiding this comment

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

It is a best practice to have each import separately than having them all together as java.io.*

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 40229a4 commit.

catch (NullPointerException | IOException e) {
throw new RuntimeException("Error reading excludingClasses.txt", e);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 40229a4 commit.

Comment on lines 342 to 357


Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 40229a4 commit.


return buff;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

There are redundant new lines used in most classes. You can check and remove them

&& createTestExecutableTask != null) {
taskBuilder.addTask(createTestExecutableTask);
}
// .addTask(new CopyResourcesTask(), listGroups) // merged with CreateJarTask
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was there before the modification. While the comment does not match with the current implementation (the code structure), this task might have some relevance?

Comment on lines 150 to 156
Path executablePath;
try {
executablePath = target.getExecutablePath(project.currentPackage()).toAbsolutePath().normalize();
} catch (IOException e) {
throw createLauncherException(e.getMessage());
}
return executablePath;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Path executablePath;
try {
executablePath = target.getExecutablePath(project.currentPackage()).toAbsolutePath().normalize();
} catch (IOException e) {
throw createLauncherException(e.getMessage());
}
return executablePath;
try {
return target.getExecutablePath(project.currentPackage()).toAbsolutePath().normalize();
} catch (IOException e) {
throw createLauncherException(e.getMessage());
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 40229a4 commit.

Comment on lines 114 to 116
if (entry.getName().endsWith(".class")) {
excludingClassPaths.add(entry.getName().replace("/", ".")
.replace(".class", ""));
Copy link
Contributor

Choose a reason for hiding this comment

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

Can use constants here. Check other places as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 40229a4 commit.

}

//create the single fat jar for all the test modules that includes the test suite json
EmitResult result = jBallerinaBackend.emit(
Copy link
Contributor

Choose a reason for hiding this comment

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

After a recent fix it was decided that the EmitResult diagnostics would only contain diagnostics reported in the executable emitting phase while JBallerinaBackend diagnostics would contain diagnostics relevant to all phases of the build.
You can check #42131 to find these changes and make changes here appropriately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

made the changes in 02decdf commit.

@@ -326,6 +336,11 @@ public void execute() {
"flag is not set");
}

if (!project.buildOptions().cloud().isEmpty() && project.buildOptions().codeCoverage()) {
this.outStream.println("WARNING: Code coverage generation is not supported with Ballerina cloud test");
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's test how --cloud=k8s is handled too.
We have to issue a warning saying tests are only supported with --cloud=docker

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will work on that. After issuing the warning, should we abort the execution?

Choose a reason for hiding this comment

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

Either we need abort or skip the cloud flag and run normal tests IMO

// compile the modules
.addTask(new CompileTask(outStream, errStream, false, isPackageModified, buildOptions.enableCache()))
Copy link
Contributor

Choose a reason for hiding this comment

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

shall we only add the Executable task addition without changing the existing implementation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By the existing implementation do you mean how these tasks are added to the TaskBuilder? Like how the Constructors are called within the addTask method?

The only issue is that, the order matters with this approach and currently CreateTestExecutableTask requires the reference to the RunTestsTask to invoke required methods. Since, RunTestsTask happens after the creation of the test executable, we cannot construct this object after the CreateTestExecutableTask using this approach.

However, currently RunTestsTask's execute method returns right after invocation if the cloud flag is enabled since we have delegated the execution of the tests to the C2C side. Therefore we do not actually need to add the RunTestsTask to the TaskBuilder. Hence, we can call RunTestsTask's constructor within CreateTestExecutableTask's constructor. Doing so, would cause readability issues. Furthermore, we also need to consider the cases where cloud flag is not given (Normal bal test execution).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After the discussion, I have made the relevant changes to reflect the previous implementation while including the current one. The change can be found in c329e24 commit.

private final transient PrintStream out;
private Path output;
private Path currentDir;
protected final transient PrintStream out;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we keep these private and add accessors if required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is done to extend this class

import static io.ballerina.cli.launcher.LauncherUtils.createLauncherException;
import static io.ballerina.projects.util.ProjectConstants.USER_DIR;

public class CreateTestExecutableTask extends CreateExecutableTask {
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the benefit of implementing the CreateExecutableTask?
IMO it's better to stick to the existing semantics of the tasks and implement Task interface instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The benefits would be the similarities between these two classes which allowed reusing some methods and properties via inheritance. Should I move back to Task implementation? In that case, I would have to write some redundant code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make logical sense for the CreateTestExecutableTask to inherit the CreateExecutableTask? AFAIU these are two classes at the same level which do similar tasks. If you want to reuse the code, perhaps we can have an abstract class/ interface that is implemented by both these classes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Other than the re usability, there are no other reasons.

Does it make logical sense for the CreateTestExecutableTask to inherit the CreateExecutableTask? AFAIU these are two classes at the same level which do similar tasks. If you want to reuse the code, perhaps we can have an abstract class/ interface that is implemented by both these classes.

That seems to be the case, I only thought from the perspective of re usability. Upon checking the code, there's one function which I moved to an utility class and other functions which I redefined in the class again. I changed the class implementation to match the structure. These new changes can be found in 73fb681 commit. Still believe there can be more improvements such as an interface like you've mentioned.

@@ -198,7 +317,6 @@ public void execute(Project project) {
continue;
}

//Set 'hasTests' flag if there are any tests available in the package
if (!hasTests) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We can remove the condition and assign hasTests=true at this point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed in 2b05eea commit.

}
}

public static boolean createTestSuiteIfHasTests(Project project, Target target,
Copy link
Contributor

@Dilhasha Dilhasha Feb 19, 2024

Choose a reason for hiding this comment

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

Suggested change
public static boolean createTestSuiteIfHasTests(Project project, Target target,
public static boolean createTestSuitesForProject(Project project, Target target,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed in 2b05eea commit.

}
}

private void performTasksAfterTestCompletion(Project project, Target target, Path testsCachePath,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
private void performTasksAfterTestCompletion(Project project, Target target, Path testsCachePath,
private void performPostTestsTasks(Project project, Target target, Path testsCachePath,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 4968e6e commit

return cmdArgs;
}

public static void addOtherNeededArgs(List<String> cmdArgs, String target, String jacocoAgentJarPath,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public static void addOtherNeededArgs(List<String> cmdArgs, String target, String jacocoAgentJarPath,
public static void appendRequiredArgs(List<String> cmdArgs, String target, String jacocoAgentJarPath,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed in 2b05eea commit.

@@ -0,0 +1,23 @@
/*
* Copyright (c) 2021, WSO2 Inc. (http://www.wso2.org) All Rights Reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

Update the license header

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 3d6d26f commit

@@ -547,6 +652,20 @@ private Path emitExecutable(Path executableFilePath) {
return executableFilePath;
}

private Path emitTest(Path executableFilePath, HashSet<JarLibrary> jarDependencies,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
private Path emitTest(Path executableFilePath, HashSet<JarLibrary> jarDependencies,
private Path emitTestExecutable(Path executableFilePath, HashSet<JarLibrary> jarDependencies,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed in 2b05eea commit.

assembleTestExecutableJar(executableFilePath, manifest, jarDependencies, testSuiteJsonPath, jsonCopyPath,
excludingClassPaths, classPathTextCopyPath);
} catch (IOException e) {
throw new ProjectException("error while creating the executable jar file for package '" +
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
throw new ProjectException("error while creating the executable jar file for package '" +
throw new ProjectException("error while creating the test executable jar file for package '" +

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 2b05eea commit

addTestExecutionDependencies(module, jarResolver, testSuite);
}

// TODO: Remove redundancy in addUtilityFunctions
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's mention this in the issue and keep track.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This TODO was already there before the modification


try (BufferedReader br = Files.newBufferedReader(testSuiteCachePath, StandardCharsets.UTF_8)) {
try (br) {
Copy link
Contributor

Choose a reason for hiding this comment

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

LEt's use try with resources

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in b26b6e5 commit

@@ -101,4 +101,18 @@ public class TesterinaConstants {
public static final int IDENTIFIER_START_INDEX = 1;
public static final int IDENTIFIER_END_INDEX = 5;

//class to identify the indices of the BTestMain args
public static class RunTimeArgs {
public static final int IS_FAT_JAR_EXECUTION = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Where are these being used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This class is used in C2C side. Specifically addDockerTestCMDArgs function in line no 344 of DockerGenerator.java

CreateTestExecutableTask createTestExecutableTask = null;

if (!project.buildOptions().cloud().isEmpty()) {
//if cloud flag is set, create the test executable
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
//if cloud flag is set, create the test executable
// if cloud flag is set, create the test executable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 40229a4 commit.

public static final int SINGLE_EXEC_TESTS = 8;
public static final int IS_RERUN_TEST_EXECUTION = 9;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 9fe35ce commit

@Override
public void execute(Project project) {
this.out.println();

Copy link
Member

Choose a reason for hiding this comment

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

Remove new line here. Check other places as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in c34f1ee commit

@@ -101,4 +101,18 @@ public class TesterinaConstants {
public static final int IDENTIFIER_START_INDEX = 1;
public static final int IDENTIFIER_END_INDEX = 5;

//class to identify the indices of the BTestMain args
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is a public class, let's have a documentation comment similar to TesterinaConstants class.

/**
 * RuntimeArgs identifies the indices of the BTestMain arg
 *
 * @since 2201.9.0
 */

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added in 9fe35ce commit

import static io.ballerina.projects.util.ProjectConstants.BLANG_COMPILED_JAR_EXT;
import static io.ballerina.projects.util.ProjectConstants.USER_DIR;

public class CreateTestExecutableTask implements Task {

Choose a reason for hiding this comment

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

Add class level comments

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

@Ramith-D-Rodrigo
Copy link
Contributor Author

Changes have been done after the code review. Addressed the suggestions and added new code segments.

  • Rebased the master (Not the latest anymore).
  • Added a new task called "CleanTargetBinTestsDirTask" to clean previous bal test --cloud=docker results (the fat jar).
  • Added relevant test cases in TestCommandTest by mocking the cloud package.
  • After the rebase, there's a runtime error when the project has mocking. This doesn't occur when the --graalvm option is given as the executing fat jar already contains the modified classes. Fixed this by reverting the orderTests()'s implementation from query action to foreach loop.

@Ramith-D-Rodrigo
Copy link
Contributor Author

Fixed the build tests failure and the strand dump test failure. Strand dump test failure was due to the change in execute.bal (using foreach instead of query action).

@Ramith-D-Rodrigo
Copy link
Contributor Author

Relevant C2C repo changes PR : ballerina-platform/module-ballerina-c2c#765

Copy link

github-actions bot commented May 1, 2024

This PR has been open for more than 15 days with no activity. This will be closed in 3 days unless the stale label is removed or commented.

@github-actions github-actions bot added the Stale label May 1, 2024
@Ramith-D-Rodrigo
Copy link
Contributor Author

Still testing the Full build pipeline

@github-actions github-actions bot removed the Stale label May 2, 2024
Copy link

codecov bot commented May 9, 2024

Codecov Report

Attention: Patch coverage is 48.82698% with 349 lines in your changes are missing coverage. Please review.

Project coverage is 77.18%. Comparing base (dca7201) to head (852d2be).
Report is 60 commits behind head on master.

Files Patch % Lines
.../main/java/io/ballerina/cli/utils/NativeUtils.java 11.23% 156 Missing and 2 partials ⚠️
...o/ballerina/cli/task/CreateTestExecutableTask.java 42.47% 97 Missing and 10 partials ⚠️
.../main/java/io/ballerina/cli/task/RunTestsTask.java 43.33% 30 Missing and 4 partials ⚠️
...rc/main/java/io/ballerina/cli/utils/TestUtils.java 84.54% 8 Missing and 9 partials ⚠️
.../java/io/ballerina/projects/JBallerinaBackend.java 88.23% 5 Missing and 3 partials ⚠️
...rc/main/java/io/ballerina/cli/cmd/TestCommand.java 57.14% 3 Missing and 3 partials ⚠️
...a/io/ballerina/projects/internal/model/Target.java 45.45% 5 Missing and 1 partial ⚠️
...va/io/ballerina/cli/task/CreateExecutableTask.java 75.00% 4 Missing ⚠️
...ballerina/runtime/internal/launch/LaunchUtils.java 0.00% 3 Missing ⚠️
...ballerina/cli/task/CleanTargetBinTestsDirTask.java 71.42% 2 Missing ⚠️
... and 3 more
Additional details and impacted files
@@             Coverage Diff              @@
##             master   #42183      +/-   ##
============================================
+ Coverage     77.13%   77.18%   +0.04%     
- Complexity    51182    51270      +88     
============================================
  Files          2924     2929       +5     
  Lines        203973   204321     +348     
  Branches      26604    26650      +46     
============================================
+ Hits         157338   157705     +367     
+ Misses        38122    38064      -58     
- Partials       8513     8552      +39     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@Dilhasha Dilhasha left a comment

Choose a reason for hiding this comment

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

LGTM

@azinneera azinneera merged commit 89f1a39 into ballerina-platform:master May 10, 2024
17 of 18 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.

[Improvement]: Introduce a way to run tests in a docker container
7 participants