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

Bring test-ci-only back #180

Merged
merged 26 commits into from
Mar 10, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
1a1d679
Revert "Revert me: Remove `--all-features` (#114)"
cmichi Feb 18, 2021
2926b4d
Pass verbosity flag
cmichi Feb 18, 2021
271696e
Format use
cmichi Feb 18, 2021
0c36a12
Run tests without `binaryen-as-dependency`
cmichi Feb 18, 2021
1b2cb32
Update to `scale-info` 0.6
cmichi Feb 18, 2021
55ecc69
Merge branch 'master' into cmichi-bring-test-ci-only-back
cmichi Feb 19, 2021
4f24000
Merge branch 'master' into cmichi-bring-test-ci-only-back
cmichi Mar 2, 2021
4643627
Fix `value used after move`
cmichi Mar 2, 2021
fed87ab
Merge branch 'master' into cmichi-bring-test-ci-only-back
cmichi Mar 3, 2021
9254d0d
Do not continue processing wasm on `check`
cmichi Mar 5, 2021
16a3f38
Make casing consistent for `log` messages
cmichi Mar 5, 2021
67d1101
Clarify behavior of `check`
cmichi Mar 5, 2021
d2bf1dd
Revert me: add debug output
cmichi Mar 5, 2021
723c9b2
Upgrade `cargo_metadata` to 0.13.1
cmichi Mar 5, 2021
6257e49
Fix assert for target path `target/ink`
cmichi Mar 5, 2021
cc82610
Revert "Revert me: add debug output"
cmichi Mar 5, 2021
7e378be
Revert "Upgrade `cargo_metadata` to 0.13.1"
cmichi Mar 5, 2021
56c488f
Merge branch 'master' into cmichi-bring-test-ci-only-back
cmichi Mar 9, 2021
c9c4d09
Merge branch 'master' into cmichi-bring-test-ci-only-back
cmichi Mar 10, 2021
49319f8
Run tests only with `binaryen-as-dependency`
cmichi Mar 10, 2021
02bc6f8
Upgrade cargo-metadata and fix usages (#210)
ascjones Mar 9, 2021
c00e7c6
Only allow new contract names beginning with an alphabetic character …
ascjones Mar 9, 2021
d8827bc
Run tests only with `binaryen-as-dependency`
cmichi Mar 10, 2021
d2720c1
Merge branch 'cmichi-bring-test-ci-only-back' of github.com:paritytec…
cmichi Mar 10, 2021
defe20d
Refactor
cmichi Mar 10, 2021
81bdd0a
Revert "Refactor"
cmichi Mar 10, 2021
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
4 changes: 1 addition & 3 deletions .gitlab-ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -79,9 +79,7 @@ test:
stage: test
<<: *docker-env
script:
# We are temporarily removing `--all-features` here for the build to succeed
# until our substrate dependencies are released in newer versions.
- cargo test --verbose --workspace
- cargo test --verbose --workspace --all-features

#### stage: build (default features)

Expand Down
40 changes: 26 additions & 14 deletions src/cmd/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -430,6 +430,9 @@ pub(crate) fn execute_with_crate_metadata(
"Building cargo project".bright_green().bold()
);
build_cargo_project(&crate_metadata, build_artifact, verbosity, unstable_flags)?;
if build_artifact == BuildArtifacts::CheckOnly {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this from some other PR?

return Ok((None, None));
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This early return is needed because the next step would otherwise try to post-process the contract. But check does not actually create a contract.

Without this change cargo contract check fails with

 [2/2] Post processing wasm file
ERROR: Loading original wasm file '/tmp/foobar/target/ink/wasm32-unknown-unknown/release/foobar.wasm'

Caused by:
    Can't read from the file: Os { code: 2, kind: NotFound, message: "No such file or directory" }

maybe_println!(
verbosity,
" {} {}",
Expand All @@ -456,7 +459,9 @@ pub(crate) fn execute_with_crate_metadata(
#[cfg(feature = "test-ci-only")]
#[cfg(test)]
mod tests_ci_only {
use crate::{cmd, util::tests::with_tmp_dir, BuildArtifacts, ManifestPath, UnstableFlags};
use crate::{
cmd, util::tests::with_tmp_dir, BuildArtifacts, ManifestPath, UnstableFlags, Verbosity,
};

#[test]
fn build_template() {
Expand All @@ -466,30 +471,33 @@ mod tests_ci_only {
ManifestPath::new(&path.join("new_project").join("Cargo.toml")).unwrap();
let res = super::execute(
&manifest_path,
None,
Verbosity::Default,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a required change, actually the code does not build without it. We just didn't notice because the test-ci-only feature was not included in the CI.

true,
BuildArtifacts::All,
UnstableFlags::default(),
)
.expect("build failed");

// we can't use `/target/ink` here, since this would match
// for `/target` being the root path. but since `ends_with`
// always matches whole path components we can be sure
// the path can never be e.g. `foo_target/ink` -- the assert
// would fail for that.
assert!(res.target_directory.ends_with("target/ink"));
assert!(res.optimization_result.unwrap().optimized_size > 0.0);
// our ci has set `CARGO_TARGET_DIR` to cache artifacts.
// this dir does not include `/target/` as a path, hence
// we can't match for e.g. `foo_project/target/ink`.
//
// we also can't match for `/ink` here, since this would match
// for `/ink` being the root path.
assert!(res.target_directory.ends_with("ink"));

let optimized_size = res.optimization_result.unwrap().optimized_size;
assert!(optimized_size > 0.0);

// our optimized contract template should always be below 3k.
assert!(res.optimization_result.unwrap().optimized_size < 3.0);
assert!(optimized_size < 3.0);

Ok(())
})
}

#[test]
fn check_must_not_create_target_in_project_dir() {
fn check_must_not_output_contract_artifacts_in_project_dir() {
with_tmp_dir(|path| {
// given
cmd::new::execute("new_project", Some(path)).expect("new project creation failed");
Expand All @@ -499,7 +507,7 @@ mod tests_ci_only {
// when
super::execute(
&manifest_path,
None,
Verbosity::Default,
true,
BuildArtifacts::CheckOnly,
UnstableFlags::default(),
Expand All @@ -508,8 +516,12 @@ mod tests_ci_only {

// then
assert!(
!project_dir.join("target").exists(),
"found target folder in project directory!"
!project_dir.join("target/ink/new_project.contract").exists(),
"found contract artifact in project directory!"
);
assert!(
!project_dir.join("target/ink/new_project.wasm").exists(),
"found wasm artifact in project directory!"
);
Ok(())
})
Expand Down
4 changes: 2 additions & 2 deletions src/cmd/metadata.rs
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,7 @@ mod tests {
use crate::cmd::metadata::blake2_hash;
use crate::{
cmd, crate_metadata::CrateMetadata, util::tests::with_tmp_dir, BuildArtifacts,
ManifestPath, UnstableFlags,
ManifestPath, UnstableFlags, Verbosity,
};
use contract_metadata::*;
use serde_json::{Map, Value};
Expand Down Expand Up @@ -379,7 +379,7 @@ mod tests {
let crate_metadata = CrateMetadata::collect(&test_manifest.manifest_path)?;
let dest_bundle = cmd::metadata::execute(
&test_manifest.manifest_path,
None,
Verbosity::Default,
BuildArtifacts::All,
UnstableFlags::default(),
)?
Expand Down
2 changes: 1 addition & 1 deletion src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -315,7 +315,7 @@ enum Command {
/// Command has been deprecated, use `cargo contract build` instead
#[structopt(name = "generate-metadata")]
GenerateMetadata {},
/// Check that the code builds as Wasm; does not output any build artifact to the top level `target/` directory
/// Check that the code builds as Wasm; does not output any `<name>.contract` artifact to the `target/` directory
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We invoke cargo check with --target-dir. This actually results in a target folder in the target-dir.

--target-dir directory
Directory for all generated artifacts and intermediate files.

You can verify it locally:

$ cargo new foo
     Created binary (application) `foo` package
$ cd foo
$ cargo check
ls
    Checking foo v0.1.0 (/tmp/foo)
    Finished dev [unoptimized + debuginfo] target(s) in 2.10s
$ ls
Cargo.lock  Cargo.toml  src  target

#[structopt(name = "check")]
Check(CheckCommand),
/// Test the smart contract off-chain
Expand Down
2 changes: 1 addition & 1 deletion src/util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ where
Verbosity::Default => &mut cmd,
};

log::info!("invoking cargo: {:?}", cmd);
log::info!("Invoking cargo: {:?}", cmd);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just for consistency with the other log messages, which start uppercase.


let child = cmd
// capture the stdout to return from this function as bytes
Expand Down