test: fix test failure in case of different default branch by amritghimire · Pull Request #6857 · treeverse/dvc

amritghimire · 2021-10-24T14:16:57Z

When the git config has set global init.defaultBranch different than master,
a few test cases were failing.
Currently, the test case is assuming the default branch to be set as
master.

This commit will introduce initial_branch option to git init call
which will ensure master branch is used as default for out test cases.

❗ I have followed the Contributing to DVC checklist.
📖 If this PR requires documentation updates, I have created a separate PR (or issue, at least) in dvc.org and linked it here.

Fixes #6043.

Thank you for the contribution - we'll try to review it as soon as possible. 🙏

efiop · 2021-10-24T17:18:53Z

Hi @amritghimire ! Nice catch!

Btw, would it make sense to maybe patch/reset global config before running all of our tests? Or could there be some important config options that might be required to run our tests? Just worried that someone will introduce a new test and will not know to put initial_branch="master" and we will not know about it until it breaks for someone using a custom default branch name.

amritghimire · 2021-10-25T03:37:46Z

Hi @amritghimire ! Nice catch!

Btw, would it make sense to maybe patch/reset global config before running all of our tests? Or could there be some important config options that might be required to run our tests? Just worried that someone will introduce a new test and will not know to put initial_branch="master" and we will not know about it until it breaks for someone using a custom default branch name.

@efiop , most of the test cases has used git_init method from dir_helpers.py to initialize the repository since it handles the retry feature as well. There was one exception to the the case in test_import, hence I changed the initialization there to include the initial_branch feature.

But yes, as you mentioned, it would be good to patch or reset the global config itself. I will look into it.

amritghimire · 2021-10-25T12:16:09Z

@efiop I have introduced config change in global for init.defaultBranch in case of CI so that for future tests with missing initial_branch will be caught with Error in CI.

amritghimire · 2021-10-27T05:36:36Z

I have introduced the isolation for home directory and root as suggested by @skshetry .
cc. @efiop @skshetry

karajan1001 · 2021-10-29T10:04:07Z

Switch to master branch still Got Anybody else also has this problem?

FAILED tests/func/test_version.py::test_ - AssertionError: assert None
FAILED tests/unit/test_info.py::test_remotes_empty - AssertionError: assert 'Remotes: None' in 'DVC version: 2.8.2.dev35+g46462080.d20211021 \n---------------------------------\nPlatform: Python 3.8.8 on macOS...
FAILED tests/unit/test_info.py::test_remotes - AssertionError: assert None

skshetry · 2021-11-18T13:03:20Z

@amritghimire ping. Let us know if you are available, we can take over this PR otherwise. 🙂

skshetry · 2021-11-19T13:20:02Z

I ran into a few issues with the isolation, need to investigate. Most likely this is an issue with our tests how some of the tests don't use dvc or tmp_dir fixtures at all.

FAILED tests/func/test_ls.py::test_ls_remote_git_only_repo_recursive - dvc.config.ConfigError: config file error: expected 'url' for dictionary value @ data['remot...
FAILED tests/func/test_ls.py::test_subrepo[False-git_dir] - dvc.config.ConfigError: config file error: expected 'url' for dictionary value @ data['remote']['minio']
FAILED tests/unit/command/test_add.py::test_add_to_remote - dvc.config.ConfigError: config file error: expected 'url' for dictionary value @ data['remote']['minio']
FAILED tests/unit/command/test_add.py::test_add_to_remote_invalid_combinations - dvc.config.ConfigError: config file error: expected 'url' for dictionary value @ d...
FAILED tests/unit/command/test_add.py::test_add_to_cache_invalid_combinations - dvc.config.ConfigError: config file error: expected 'url' for dictionary value @ da...
FAILED tests/unit/command/test_data_sync.py::test_pull - dvc.config.ConfigError: config file error: expected 'url' for dictionary value @ data['remote']['minio']
FAILED tests/unit/command/test_data_sync.py::test_push - dvc.config.ConfigError: config file error: expected 'url' for dictionary value @ data['remote']['minio']
FAILED tests/unit/command/test_diff.py::test_default - dvc.config.ConfigError: config file error: expected 'url' for dictionary value @ data['remote']['minio']
FAILED tests/unit/command/test_diff.py::test_show_hash - dvc.config.ConfigError: config file error: expected 'url' for dictionary value @ data['remote']['minio']
FAILED tests/unit/command/test_diff.py::test_show_json - dvc.config.ConfigError: config file error: expected 'url' for dictionary value @ data['remote']['minio']
FAILED tests/unit/command/test_diff.py::test_show_json_and_hash - dvc.config.ConfigError: config file error: expected 'url' for dictionary value @ data['remote']['...
FAILED tests/unit/command/test_diff.py::test_show_json_hide_missing - dvc.config.ConfigError: config file error: expected 'url' for dictionary value @ data['remote...
FAILED tests/unit/command/test_diff.py::test_diff_show_markdown_and_hash[None] - dvc.config.ConfigError: config file error: expected 'url' for dictionary value @ d...
FAILED tests/unit/command/test_diff.py::test_diff_show_markdown_and_hash[True] - dvc.config.ConfigError: config file error: expected 'url' for dictionary value @ d...
FAILED tests/unit/command/test_diff.py::test_diff_show_markdown_and_hash[False] - dvc.config.ConfigError: config file error: expected 'url' for dictionary value @ ...
FAILED tests/unit/command/test_diff.py::test_no_changes - dvc.config.ConfigError: config file error: expected 'url' for dictionary value @ data['remote']['minio']
FAILED tests/unit/command/test_diff.py::test_hide_missing - dvc.config.ConfigError: config file error: expected 'url' for dictionary value @ data['remote']['minio']
FAILED tests/unit/command/test_experiments.py::test_experiments_diff_revs - dvc.config.ConfigError: config file error: expected 'url' for dictionary value @ data['...
FAILED tests/unit/command/test_data_sync.py::test_fetch - dvc.config.ConfigError: config file error: expected 'url' for dictionary value @ data['remote']['minio']
FAILED tests/unit/command/test_imp.py::test_import - dvc.config.ConfigError: config file error: expected 'url' for dictionary value @ data['remote']['minio']
FAILED tests/unit/command/test_imp.py::test_import_no_exec - dvc.config.ConfigError: config file error: expected 'url' for dictionary value @ data['remote']['minio']
FAILED tests/unit/command/test_imp_url.py::test_import_url - dvc.config.ConfigError: config file error: expected 'url' for dictionary value @ data['remote']['minio']
FAILED tests/unit/command/test_imp_url.py::test_failed_import_url - dvc.config.ConfigError: config file error: expected 'url' for dictionary value @ data['remote']...
FAILED tests/unit/command/test_imp_url.py::test_import_url_no_exec - dvc.config.ConfigError: config file error: expected 'url' for dictionary value @ data['remote'...
FAILED tests/unit/command/test_imp_url.py::test_import_url_to_remote - dvc.config.ConfigError: config file error: expected 'url' for dictionary value @ data['remot...
FAILED tests/unit/command/test_imp_url.py::test_import_url_to_remote_invalid_combination - dvc.config.ConfigError: config file error: expected 'url' for dictionary...

So, I have some global dvc configs, and it's causing it to fail somehow, which it should not have been able to see. It worked fine before.

amritghimire · 2021-11-20T07:06:04Z

@skshetry Can you check if the config file use absolute path for config or something to fetch the global dvc config?

skshetry · 2021-12-08T11:53:46Z

For the record, I have taken over this PR. Thanks @amritghimire, we'll take it from here. 🙂

skshetry · 2021-12-14T15:34:51Z

For the record, we may just end up isolating git for the first part as done in https://github.com/iterative/scmrepo/blob/9dcd9061cf5e529ebddca5ff9056d26fae1372f6/tests/conftest.py#L37-L62 stolen from this PR.

Isolating dvc's global/system config can be done separately.

When the git config has set global `init.defaultBranch` different than master, a few test cases were failing. Currently, the test case is assuming the default branch to be set as master. This commit will introduce **initial_branch** option to git init call which will ensure *master* branch is used as default for out test cases. Set default git branch to main for test CI Since main is going to be new default branch, it will let the test case handle the scenario where default branch is set other than master. Introduce isolation for git configuration

skshetry · 2022-01-06T10:51:48Z

@efiop, @dtrifiro this is ready for review, let me know if this solves the issue. 🙂

skshetry · 2022-01-06T12:48:42Z

@dtrifiro, did this fix the issue that you were having?

skshetry · 2022-01-07T05:27:23Z

Merging this, thanks @amritghimire.

Note that this only provides isolation for git, DVC's own global/system configs are not isolated yet. :)

efiop · 2022-01-07T09:58:54Z

@amritghimire @skshetry thank you guys, outstanding work! 🔥

amritghimire requested a review from a team as a code owner October 24, 2021 14:16

amritghimire requested a review from pared October 24, 2021 14:16

efiop requested review from efiop and removed request for pared October 24, 2021 17:19

efiop added testing Related to the tests and the testing infrastructure bug Did we break something? bugfix fixes bug and removed bug Did we break something? labels Oct 24, 2021

skshetry reviewed Oct 25, 2021

View reviewed changes

Comment thread .github/workflows/tests.yaml Outdated

skshetry reviewed Oct 25, 2021

View reviewed changes

Comment thread tests/func/test_import.py Outdated

efiop requested a review from skshetry October 28, 2021 20:26

skshetry reviewed Nov 3, 2021

View reviewed changes

Comment thread tests/conftest.py Outdated

skshetry reviewed Nov 3, 2021

View reviewed changes

Comment thread tests/conftest.py Outdated

skshetry added the awaiting response we are waiting for your reply, please respond! :) label Nov 15, 2021

skshetry mentioned this pull request Nov 18, 2021

Some git-related tests fail when defaultBranch is not master #6998

Closed

skshetry mentioned this pull request Nov 19, 2021

tests: move away from use of hardcoded master #7005

Closed

skshetry added ci skip-changelog Skips changelog and removed awaiting response we are waiting for your reply, please respond! :) labels Nov 19, 2021

skshetry self-assigned this Dec 8, 2021

skshetry requested review from dtrifiro and skshetry and removed request for skshetry January 5, 2022 15:18

try fixing windows

d3c3e6f

dtrifiro reviewed Jan 6, 2022

View reviewed changes

Comment thread tests/conftest.py

dtrifiro approved these changes Jan 6, 2022

View reviewed changes

skshetry merged commit 44a1146 into treeverse:main Jan 7, 2022

meshde mentioned this pull request Sep 18, 2022

dvcignore: read ignore patterns from global .dvcignore #8280

Closed

2 tasks

Conversation

amritghimire commented Oct 24, 2021 • edited by skshetry Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

efiop commented Oct 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amritghimire commented Oct 25, 2021

Uh oh!

amritghimire commented Oct 25, 2021

Uh oh!

Uh oh!

Uh oh!

amritghimire commented Oct 27, 2021

Uh oh!

karajan1001 commented Oct 29, 2021

Uh oh!

Uh oh!

Uh oh!

skshetry commented Nov 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skshetry commented Nov 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amritghimire commented Nov 20, 2021

Uh oh!

skshetry commented Dec 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skshetry commented Dec 14, 2021

Uh oh!

skshetry commented Jan 6, 2022

Uh oh!

Uh oh!

skshetry commented Jan 6, 2022

Uh oh!

skshetry commented Jan 7, 2022

Uh oh!

efiop commented Jan 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

amritghimire commented Oct 24, 2021 •

edited by skshetry

Loading

efiop commented Oct 24, 2021 •

edited

Loading

skshetry commented Nov 18, 2021 •

edited

Loading

skshetry commented Nov 19, 2021 •

edited

Loading

skshetry commented Dec 8, 2021 •

edited

Loading