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

Testing 2/3: Acceptance test framework #142

Merged
merged 24 commits into from
Oct 12, 2018

Conversation

mikrostew
Copy link
Contributor

@mikrostew mikrostew commented Sep 4, 2018

This is a huge PR, with 2K+ lines of code added. The main change is being able to run integration tests with notion in a sandboxed environment, with tests like this:

#[test]
fn use_node() {
    let s = sandbox()
        .package_json(BASIC_PACKAGE_JSON)
        .build();

    assert_that(
        s.notion("use node 10"),
        execs()
            .with_status(0)
            .with_stdout_contains("Pinned node to version 10.8.0 in package.json"),
    );

    assert_eq!(
        s.read_package_json(),
        package_json_with_pinned_node("10.8.0"),
    )
}

Overview of these changes

Added network mocking with mockito, using some forked changes to make interoperating with reqwest on windows-64 work.

Adapted code from the testing framework in cargo, which does some similar things to what we need. Most of these needed some adjustment to work with Notion (mostly using Notion errors), and some #[allow(dead_code)] annotations to avoid getting a ton of warnings for things that aren't currently being used.

  • hamcrest.rs - a subset of hamcrest implementing things used in the test framework
  • matchers.rs - functions used with hamcrest, to match the stdout/stderr and other things from a process
  • paths.rs - creates and manipulates the testing directories
  • process.rs - functions to build a process and set its environment
  • sandbox.rs - this has a few things from cargo, but most of this is Notion-specific code to setup the files and directories, configure the environment, and mock network calls

Wrote some tests for notion use, notion current, and notion deactivate.

Moved package.json to the dev/ directory. Because the test environments are created in target/, if there is no package.json created for the test (for scenarios that test what happens outside of a project), then Notion would recurse up the file tree and use the package.json in the root of the repo, which broke the tests.

Next steps

To actually test downloading and installing archive files, this needs to use a real http server. mockito only works with responses that are valid UTF-8 strings, and cannot handle files. Right now I'm faking some things for the HEAD and Accept: Range requests, but in the future these should actually be handled correctly.

Long-term plan

Since so much of this code is shared with cargo, and other projects would benefit from using this framework, we should extract as much as possible to a separate library crate, and use that.

Closes #7.

@mikrostew mikrostew force-pushed the acceptance-tests branch 2 times, most recently from a6e202d to 374058f Compare September 7, 2018 21:09
@mikrostew mikrostew changed the title [WIP] Acceptance test framework Testing 2/3: Acceptance test framework Sep 10, 2018
Copy link
Collaborator

@dherman dherman left a comment

Choose a reason for hiding this comment

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

OK I did my best to review the whole thing. I'm not sure I've followed 100% of it, but it'll help when I make changes and write tests of my own.

tests/testsuite/support/hamcrest.rs Outdated Show resolved Hide resolved
.travis.yml Show resolved Hide resolved
crates/notion-core/Cargo.toml Outdated Show resolved Hide resolved
@@ -164,12 +170,16 @@ pub fn shim_file(toolname: &str) -> Fallible<PathBuf> {
// catalog.toml user_catalog_file

fn local_data_root() -> Fallible<PathBuf> {
#[cfg(windows)]
return Ok(winfolder::Folder::LocalAppData.path().join("Notion"));
if let Some(home_dir) = env::home_dir() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the reason for this? I don't follow the logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm using this to sandbox the test environment, since otherwise the $HOME\AppData\Local\ folder will be accessed directly, under the user that is running the tests instead of the home directory in the sandbox.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We talked on the phone this morning and came up with a strategy. Here's a synopsis:

  • We'll make a NOTION_SANDBOX environment variable to make it clearer when our paths are being computed for a sandboxed root directory. Our acceptance tests are not testing the standard directories, but that's true no matter what; this just makes the logic easier to follow and maintain.
  • In a separate PR, we'll make one or two smoke tests that require a global testing lock, clear out the Notion directories and reset them to a default state, and just test a few basic workflows. (We should file an issue for this.)
  • I'll add some unit tests to the winfolder crate.

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 an issue for smoke tests: #162

tests/testsuite/main.rs Outdated Show resolved Hide resolved
tests/testsuite/main.rs Outdated Show resolved Hide resolved
tests/testsuite/support/matchers.rs Outdated Show resolved Hide resolved
tests/testsuite/support/matchers.rs Outdated Show resolved Hide resolved
tests/testsuite/support/matchers.rs Outdated Show resolved Hide resolved
tests/testsuite/support/sandbox.rs Outdated Show resolved Hide resolved
@mikrostew
Copy link
Contributor Author

Rebased to pull in recent changes, since this PR has been open for like a month now

@dherman
Copy link
Collaborator

dherman commented Oct 12, 2018

Thanks for keeping at it! The env var helps with readability. There may still be improvements we can do but we need to land this and keep moving.

We should chat when we have a chance about how we want to go about merging efforts with cargo. @mikrostew maybe when you have a chance you could write a bullet list of the pieces of cargo you'd want to see extracted, and any changes or extension points you need to make that work? And then we can reach out to the cargo maintainers.

@dherman dherman merged commit 05b6b79 into volta-cli:master Oct 12, 2018
@mikrostew mikrostew deleted the acceptance-tests branch December 13, 2018 17:58
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.

None yet

2 participants