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

CI: Tweaks for backend tests #9001

Merged
merged 4 commits into from
Feb 12, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 13 additions & 10 deletions build/build/src/engine/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -394,15 +394,23 @@ impl RunContext {

// === Unit tests and Enso tests ===
debug!("Running unit tests and Enso tests.");
if self.config.test_scala {
// We store Scala test result but not immediately fail on it, as we want to run all the
// tests (including standard library ones) even if Scala tests fail.
let scala_test_result = if self.config.test_scala {
// Make sure that `sbt buildEngineDistributionNoIndex` is run before
// `project-manager/test`. Note that we do not have to run
// `buildEngineDistribution` (with indexing), because it is unnecessary.
sbt.call_arg("buildEngineDistributionNoIndex").await?;

// Run unit tests
sbt.call_arg("set Global / parallelExecution := false; test").await?;
}
sbt.call_arg("set Global / parallelExecution := false; test").await.inspect_err(|e| {
ide_ci::actions::workflow::error(format!("Scala Tests failed: {e:?}"))
})
} else {
// No tests - no fail.
Ok(())
};

if self.config.test_standard_library {
enso.run_tests(IrCaches::No, &sbt, PARALLEL_ENSO_TESTS).await?;
}
Expand Down Expand Up @@ -540,13 +548,6 @@ impl RunContext {
// Verify License Packages in Distributions
// FIXME apparently this does not work on Windows due to some CRLF issues?
if self.config.verify_packages && TARGET_OS != OS::Windows {
/* refversion=${{ env.ENSO_VERSION }}
binversion=${{ env.DIST_VERSION }}
engineversion=$(${{ env.ENGINE_DIST_DIR }}/bin/enso --version --json | jq -r '.version')
test $binversion = $refversion || (echo "Tag version $refversion and the launcher version $binversion do not match" && false)
test $engineversion = $refversion || (echo "Tag version $refversion and the engine version $engineversion do not match" && false)
*/

for package in ret.packages() {
package.verify_package_sbt(&sbt).await?;
}
Expand Down Expand Up @@ -588,6 +589,8 @@ impl RunContext {
bundle.create(&self.repo_root, &graal_version).await?;
}

scala_test_result?;

Ok(ret)
}

Expand Down
10 changes: 5 additions & 5 deletions build/build/src/enso.rs
Original file line number Diff line number Diff line change
Expand Up @@ -80,13 +80,12 @@ impl BuiltEnso {
.await
}

pub fn run_test(&self, test: impl AsRef<Path>, ir_caches: IrCaches) -> Result<Command> {
let test_path = self.paths.repo_root.test.join(test);
pub fn run_test(&self, test_path: impl AsRef<Path>, ir_caches: IrCaches) -> Result<Command> {
let mut command = self.cmd()?;
command
.arg(ir_caches)
.arg("--run")
.arg(test_path)
.arg(test_path.as_ref())
// This flag enables assertions in the JVM. Some of our stdlib tests had in the past
// failed on Graal/Truffle assertions, so we want to have them triggered.
.set_env(JAVA_OPTS, &ide_ci::programs::java::Option::EnableAssertions.as_ref())?;
Expand Down Expand Up @@ -157,8 +156,9 @@ impl BuiltEnso {
_ => None,
};

let futures = crate::paths::LIBRARIES_TO_TEST.map(ToString::to_string).map(|test| {
let command = self.run_test(test, ir_caches);
let std_tests = crate::paths::discover_standard_library_tests(&paths.repo_root)?;
let futures = std_tests.into_iter().map(|test_path| {
let command = self.run_test(&test_path, ir_caches);
async move { command?.run_ok().await }
});

Expand Down
21 changes: 10 additions & 11 deletions build/build/src/paths.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,17 +42,16 @@ ide_ci::define_env_var! {

pub const EDITION_FILE_ARTIFACT_NAME: &str = "Edition File";

pub const LIBRARIES_TO_TEST: [&str; 7] = [
"Examples_Tests",
"Geo_Tests",
"Image_Tests",
// Temporarily disabled due to https://www.pivotaltracker.com/story/show/184042416
// "Meta_Test_Suite_Tests",
"Table_Tests",
"Base_Tests",
"AWS_Tests",
"Visualization_Tests",
];
/// Get paths of directories containing standard library test project.
pub fn discover_standard_library_tests(repo_root: &generated::RepoRoot) -> Result<Vec<PathBuf>> {
// The glob pattern will discover paths like "H:\NBO\enso\test\AWS_Tests\package.yaml".
let glob_pattern = "**/*_Tests/package.yaml";
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
let glob_pattern = "**/*_Tests/package.yaml";
let glob_pattern = "test/*_Tests/package.yaml";

What about the above?

I'm not sure if we need to or even want to search in other directories.

Copy link
Member

Choose a reason for hiding this comment

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

There are plans to move tests to be next to each library being tested (i.e. something like test folder next to src of each Enso library), so this may need an update then as well.

Copy link
Contributor Author

@mwu-tow mwu-tow Feb 9, 2024

Choose a reason for hiding this comment

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

What about the above?

I'm not sure if we need to or even want to search in other directories.

We attach the glob pattern to the repo_root.test which already paths to the test directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are plans to move tests to be next to each library being tested (i.e. something like test folder next to src of each Enso library), so this may need an update then as well.

Yes. This should not be a problem.

Copy link
Member

Choose a reason for hiding this comment

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

What about the above?
I'm not sure if we need to or even want to search in other directories.

We attach the glob pattern to the repo_root.test which already paths to the test directory.

In that case is the ** even needed?

Unless I'm wrong in this context, usually ** in a glob means multi-level directory traversal and I don't see why that would be needed - in our case we don't need nesting, all tests should be the immediate children of the test dir and looking 'deeper' seems like asking for trouble (if some test project contains an inner project for some reason).

That is a very minor nitpick though, as currently either way works 100% fine and we can always tweak it if needed. So feel free to ignore this suggestion.

let glob_pattern = repo_root.test.join(glob_pattern);
glob::glob(glob_pattern.as_str())?
// Package manifest path -> Parent directory.
.map(|package_path_result| Result::Ok(package_path_result?.try_parent()?.to_path_buf()))
.try_collect_vec()
}

pub fn new_repo_root(repo_root: impl Into<PathBuf>, triple: &TargetTriple) -> generated::RepoRoot {
generated::RepoRoot::new_root(
Expand Down
4 changes: 4 additions & 0 deletions build/ci_utils/src/actions/workflow.rs
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,10 @@ pub fn message(level: MessageLevel, text: impl AsRef<str>) {
Message { level, text: text.as_ref().into() }.send()
}

pub fn error(text: impl AsRef<str>) {
message(MessageLevel::Error, text)
}

pub fn warn(text: impl AsRef<str>) {
message(MessageLevel::Warning, text)
}
Loading