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

Operation ids are nondetermenistic in tests after a jj git clone. #1596

Closed
ilyagr opened this issue May 11, 2023 · 5 comments
Closed

Operation ids are nondetermenistic in tests after a jj git clone. #1596

ilyagr opened this issue May 11, 2023 · 5 comments

Comments

@ilyagr
Copy link
Contributor

ilyagr commented May 11, 2023

This issue is somewhat minor, but I find it confusing, and it slowed down my attempts to debug #864 quite a bit.

Operation ids are meant to be deterministic in tests. They usually are, as the following example shows.

Example of passing test to show that operation ids are deterministic
#[test]
fn test_debug_operation() {
    let test_env = TestEnvironment::default();
    test_env.jj_cmd_success(test_env.env_root(), &["init", "repo", "--git"]);
    let workspace_path = test_env.env_root().join("repo");
    let stdout = test_env.jj_cmd_success(&workspace_path, &["new", "-m=1"]);
    assert_snapshot!(stdout, @r###"
    Working copy now at: 472cb20289ba 1
    Parent commit      : 230dd059e1b0 (no description set)
    "###);
    let stdout = test_env.jj_cmd_success(&workspace_path, &["new", "@-", "-m=2"]);
    assert_snapshot!(stdout, @r###"
    Working copy now at: cb634f95940d 2
    Parent commit      : 230dd059e1b0 (no description set)
    "###);
    let stdout = test_env.jj_cmd_success(&workspace_path, &["op", "log"]);
    assert_snapshot!(stdout, @r###"
    @  cea83e7f7c9c [email protected] 2001-02-03 04:05:09.000 +07:00 - 2001-02-03 04:05:09.000 +07:00
    │  new empty commit
    │  args: jj new @- '-m=2'
    ◉  bbc6ddfffe69 [email protected] 2001-02-03 04:05:08.000 +07:00 - 2001-02-03 04:05:08.000 +07:00
    │  new empty commit
    │  args: jj new '-m=1'
    ◉  a99a3fd5c51e [email protected] 2001-02-03 04:05:07.000 +07:00 - 2001-02-03 04:05:07.000 +07:00
    │  add workspace 'default'
    ◉  56b94dfc38e7 [email protected] 2001-02-03 04:05:07.000 +07:00 - 2001-02-03 04:05:07.000 +07:00
       initialize repo
    "###);
}

However, they are not deterministic after a jj git clone. For example, the following test will fail
because the operation ids marked as ee79b02a33d4 (and all the following ones) will change on each run.

Test showing non-determinism after `jj git clone`
// TODO: This test fails
#[test]
fn test_determenistic_operation_ids_after_clone() {
    let test_env = TestEnvironment::default();
    let origin_path = test_env.env_root().join("origin");
    git2::Repository::init(&origin_path).unwrap();
    test_env.jj_cmd_success(&origin_path, &["init", "--git-repo=."]);
    test_env.jj_cmd_success(&origin_path, &["describe", "-m=A"]);
    test_env.jj_cmd_success(&origin_path, &["branch", "create", "A"]);
    test_env.jj_cmd_success(&origin_path, &["new", "-m=B"]);
    test_env.jj_cmd_success(&origin_path, &["branch", "create", "B"]);
    test_env.jj_cmd_success(&origin_path, &["new", "-m=C"]);

    let clone_path = test_env.env_root().join("clone");
    std::fs::create_dir(&clone_path).unwrap();
    test_env.jj_cmd_success(
        &clone_path,
        &[
            "git",
            "clone",
            origin_path.to_str().unwrap(),
            clone_path.to_str().unwrap(),
        ],
    );
    let stdout = test_env.jj_cmd_success(&clone_path, &["operation", "log"]);
    insta::assert_snapshot!(stdout, @r###"
    @  7a5cadf37896 [email protected] 2001-02-03 04:05:13.000 +07:00 - 2001-02-03 04:05:13.000 +07:00
    │  check out git remote's default branch
    │  args: jj git clone $TEST_ENV/origin $TEST_ENV/clone
    ◉  ee79b02a33d4 [email protected] 2001-02-03 04:05:13.000 +07:00 - 2001-02-03 04:05:13.000 +07:00
    │  fetch from git remote into empty repo
    │  args: jj git clone $TEST_ENV/origin $TEST_ENV/clone
    ◉  9bb4784b563b [email protected] 2001-02-03 04:05:13.000 +07:00 - 2001-02-03 04:05:13.000 +07:00
    │  add workspace 'default'
    ◉  40b0eefc6c7f [email protected] 2001-02-03 04:05:13.000 +07:00 - 2001-02-03 04:05:13.000 +07:00
       initialize repo
    "###);
 }

If we add a call to jj debug operation to that test, we can notice the following:

  1. (Red herring) The output shows two head_ids as part of the view object, and their order can change.
    This is a red herring and does not affect the operation ids (this can be seen by adding jj debug operation to the passing example test above). Moreover, even if it did, that would create
    only two different possibilities, not a distinct result on every run.
  2. The operation id can be different without any changes to jj debug operations as, for example, the diff at ilyagr@op-id-nondeterminism shows.
@martinvonz
Copy link
Member

I think that's because we include the command arguments in the operation. You can probably make the paths relative in the test.

@ilyagr
Copy link
Contributor Author

ilyagr commented May 11, 2023

Why don't they show up as different in the output to jj debug operation or jj op log?

@martinvonz
Copy link
Member

Because we normalize the paths there :)

@ilyagr
Copy link
Contributor Author

ilyagr commented May 11, 2023

Heh. 🤦‍♂️

Yup, changing the clone command to

test_env.jj_cmd_success(&clone_path, &["git", "clone", "../origin", "."]);

fixed the problem.

That was a fun few hours. I really wish we could do something better here, though I'm highly biased at the moment. The first option that comes to mind is to call normalize_output explicitly in tests when needed.

@ilyagr
Copy link
Contributor Author

ilyagr commented May 11, 2023

I'll just close this. I'm not sure how likely more people are to be confused by this.

@ilyagr ilyagr closed this as completed May 11, 2023
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

No branches or pull requests

2 participants