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

Implement prompt consistency tests and harmonize all activate scripts #1330

Merged
merged 18 commits into from Apr 10, 2019

Conversation

Projects
None yet
2 participants
@bskinn
Copy link
Contributor

commented Mar 1, 2019

See history of bskinn/virtualenv:all-prompts branch, tip at
bskinn/virtualenv@67cda5f, for the actual sequence of
development of the prompt tests, leading to the first commit of the PR (e6a2b99)

This PR will NOT be ready to merge until the activate scripts for csh, xonsh, cmd and posh have been fixed. PR created now per discussion at #1316, downstream of #1168 and #1285.

At time PR was opened, closes #1294 and closes #1295. By the time PR is complete, will close #1296, close #1297, close #1298, and close #1299. Also resolves #1168, the initial discussion that prompted all this.

Planning also to fix the minor documentation error to close #1304 before merging.

Thanks for contributing a pull request, see checklist all is good!

  • wrote descriptive pull request text
  • added/updated test(s)
  • updated/extended the documentation
  • added news fragment in docs/changelog folder
SQUASH: Implement prompt consistency tests
See history of bskinn/virtualenv:all-prompts branch, tip at
bskinn/virtualenv@67cda5f, for the actual sequence of
development
Show resolved Hide resolved setup.cfg Outdated
Show resolved Hide resolved tests/activation/test_prompts.py Outdated
Show resolved Hide resolved tests/activation/test_prompts.py Outdated

bskinn added some commits Mar 1, 2019

SQUASH (3): Various fixes and responses to review
(1) Fix pathlib import for 2/3 compat

(2) Refactor preamble_cmds to defaultdict

(3) Convert shell info to namedtuples

Also switch filename template string fields to autonumber.

Also revert addition of black to setup.cfg
Harmonize activate.bat.
All cmd tests now pass (locally, CI pending). Closes #1297.
Harmonize activate.ps1
All current PoSH tests now pass locally. Closes #1298.
Revise for 3.4, 2.7 compat
Path doesn't have .write_text() or .read_bytes() in 3.4.

2.7 complains if the environment variables are typed as unicode.

@bskinn bskinn force-pushed the bskinn:pr-all-prompts branch from 2e584ba to 01106ea Mar 1, 2019

SQUASH: Fix lint, expand DISABLE_PROMPT tests
(1) Rearrange test_prompts.py to satisfy linters

When function and variable definitions are interleaved at the module
level, black and flake8 cannot be satisfied simultaneously,
due to conflicting visions about how many blank lines should be
present at certain spots.

(2) Expand DISABLE_PROMPT tests for granularity

VIRTUAL_ENV_DISABLE_PROMPT, in bash, disables the prompt if the
env var is defined and of nonzero length, REGARDLESS of the
truthiness of the nonzero-length value. These new tests bracket
this specific behavior definition precisely.
Refactor/change per review
Switch to virtualenv.IS_WIN for platform checking.

Refactor platform pytest.skip checks to per-shell "select case"
sort of layout. Also refactor so that the actual skip occurs
in the test function, where the fixture function just returns
a skip message if skip is called for.

Refactor the PoSH executability check to its own fixture. This
speeds up the test runner by only carrying out the execution check
once.

Refactor the shell_info definitions to eliminate the
.__new__.__defaults__ hack, since namedtuple(.., defaults=[...]) is
only available on 3.7+.

No more single-character function argument in env_compat.

@bskinn bskinn force-pushed the bskinn:pr-all-prompts branch 6 times, most recently from a2afe17 to ef4cfbd Mar 11, 2019

@bskinn bskinn force-pushed the bskinn:pr-all-prompts branch 2 times, most recently from d163182 to b13242f Mar 11, 2019

Eliminate spurious Azure fail; tweak clean_env init
It appears that for a completely non-colorized terminal fish's set_color
command returns either a null, or an empty string, or something.
This leads to incorrect string interpolation on the Azure test VMs.

See:
https://github.com/pypa/virtualenv/blob/94c6bcb4a3f8d7ff63e2215b18d76514d147a762/virtualenv_embedded/activate.fish#L92

The test in question fails, but the failing value of the prompt
is "env()". This implies that the interpolation is not "seeing anything"
from the output of the "(set_color normal)" invocation, and instead
putting the environment name ahead of, rather than into, the parentheses.

Hopefully this added env variable in the preamble of the script
will coerce fish into always including colorizing ANSI codes even
on the Azure VMs.

---

Also, tweak clean_env fixture to ensure VIRTUAL_ENV_DISABLE_PROMPT is
not present, as the tests all assume it is *unset* in the clean_env.

Also also, buff a number of comments.

@bskinn bskinn force-pushed the bskinn:pr-all-prompts branch from b13242f to 58c5147 Mar 11, 2019

bskinn added some commits Mar 11, 2019

Harmonize activate.xsh.
BEAR IN MIND, this only works for xonsh 0.8.9 and newer, and only
as long as xonsh doesn't change its implementation.

Probably the xonsh tests should be skipped on CI, since changes
outside the virtualenv codebase could cause breakage.

It would still be informative to have the xonsh tests in the
codebase, though -- perhaps a pytest CLI argument could be added
to enable them?
Add pytest CLI option to not skip xonsh prompt tests
Since xonsh prompt configuration functionality is mostly handled
in the xonsh codebase, don't want changes in xonsh to suddenly
start causing virtualenv CI builds to fail.
Switch csh on MacOS to weaker test
MacOS (t)csh apparently prepends "prompt\t" to the content printed
to stdout, precluding the stricter equality test of the
prompt contents.

@bskinn bskinn force-pushed the bskinn:pr-all-prompts branch from 8170577 to fb8f0be Mar 12, 2019

@bskinn bskinn marked this pull request as ready for review Mar 12, 2019

Show resolved Hide resolved conftest.py Outdated
Show resolved Hide resolved tests/activation/test_prompts.py Outdated
Show resolved Hide resolved virtualenv_embedded/activate.ps1

bskinn added some commits Apr 6, 2019

(SQUASH 4) Refactor to shell-info class model
(1) WIP: Convert partially to info class model

Still midstream, so lots still broken.

(2) WIP: More conversion to shell-info class model

(3) Finish shell-info class conversion & cleanup.

Needs testing on Windows before merge to main PR branch.

(4) Fix posh provisioning test glitches

@bskinn bskinn force-pushed the bskinn:pr-all-prompts branch 2 times, most recently from 257d36c to caafa76 Apr 9, 2019

Fix ShellInfo for new-style py2 class
Force push to re-trigger Azure pipeline x2

@bskinn bskinn force-pushed the bskinn:pr-all-prompts branch from caafa76 to 677bbc1 Apr 9, 2019

@gaborbernat gaborbernat merged commit c787671 into pypa:master Apr 10, 2019

1 check passed

pypa.virtualenv #pypa.virtualenv_20190409.06 succeeded
Details
@gaborbernat

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2019

Thanks, @bskinn, however once merged pypy3 tests started failing - https://dev.azure.com/pypa/virtualenv/_build/results?buildId=6588 can you investigate?

@bskinn

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2019

@gaborbernat I checked it...the failing test isn't related to the new prompt test machinery. Was something changed/fixed relating to pypy3 recently? I realize now, I forgot to merge/rebase master, so the merged PR may have obliterated a recent change?

@bskinn

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2019

Hm, no -- none of the recent commits on the history seem at all related to this... if anything, it looks like a behavior change in pypy3... though it's really odd timing that it would've been changed between my committing 677bbc1 and your merging the PR.

It looks like the test was expecting pypy3 to error on an attempt to import distutils, due to some pypy3 oddity, but it did not error.

        def _check_no_warnings(module):
            subprocess.check_call((exe, "-Werror", "-c", "import {}".format(module)))
    
        # pypy3's `distutils.sysconfig_pypy` imports `imp`
        # https://bitbucket.org/pypy/pypy/pull-requests/634/remove-unused-and-deprecated-import-of-imp/diff
        if virtualenv.IS_PYPY and sys.version_info > (3,):
            with pytest.raises(subprocess.CalledProcessError):
>               _check_no_warnings("distutils")
E               Failed: DID NOT RAISE <class 'subprocess.CalledProcessError'>

I don't think this is something that I'm equipped to fix -- I don't know anything about the internals of pypy3, and I have no idea if/how this distutils behavior affects virtualenv.

@bskinn

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

I looked at it some more and... Huh. That pypy PR mentioned in the comment in the test function got merged.

Maybe this fixed the error in pypy3, and there's no reason any more for the pypy3 special-case pytest.raises(...) check?

@gaborbernat

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2019

I believe so too 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.