Skip to content

Conversation

@cderv
Copy link
Collaborator

@cderv cderv commented Jan 25, 2023

WIP: Draft PR until it is working

Follow up work

The following are related to ignored tests on Windows and should be fixed

Adaptations

  • Removing ./tests/bin/* folder
    • This folder and source bin/activate were not useful (some hardcoded miniconda path in it). Removing it seems to work ok.
  • Adding a .python_version file to pin a python version to use in tests/
    • This is understood by setup-python
    • It allows to use pyenv to automically use the right version of python in when in tests/
  • Supporting Windows in our custom action quarto-dev which install quarto development version
    • It was not an easy piece to make things work on Unix and Windows. Especially because we run a quarto.cmd, which is not found in PATH on windows's bash. Using cmd shell is a pain as it seems you can't really set %GITHUB_PATH% on GHA (🤷‍♂️ ). So for now, it is hardcoded path but won't probably change soon. Anyway it is working now.
  • Adding some caching for Julia plus adaptation so that it runs on Windows too.

Probably some more to change

  • We are setting up environment in GHA in some steps, but we are also doing renv::restore() and pip install in the run-test. It seems a duplication. I am thinking of separating config-tests-env script for configure a test environment, and then having run-test to just run the test. But still need to think through CI usage vs Local usage.

  • Having a quarto.ps1 instead of quarto.cmd would be better for dev windows user. I never used CMD, and other windows dev probably use either Powershell or Git Bash.

Notes on developing tests for Linux and windows

  • When using regex matching, newline needs to be OS independant (3452940)
  • If a tests does not need to be run on several OS (too costly to adapt, or not necessary to make OS independant), it can be easily ignored (418d077)
  • Some calls to quarto needs to be explicitly made to quarto.cmd on Windows because in dev version quarto.exe is not available and quarto without extension will not be found (6be8232)
  • Windows file locking system must be taken into account (example in cleaning where one need to move out a directory before removing it c29a182)
  • Windows paths specificity needs to be dealt with (example with path ending / vs \ 9f37289)

More comments to come as I go through with it

@dragonstyle
Copy link
Collaborator

I would like to note that this is an extremely exciting PR.

cderv added 5 commits January 25, 2023 23:46
We duplicate the step in the `run-test` script, but it is better to have this setup before installing quarto-dev in next step
* Using tmate
* Deactivating other workflow as not useful when debugging
@cderv cderv force-pushed the windows/run-tests branch from 2ad731a to 3bb301a Compare January 26, 2023 10:46
@cderv cderv force-pushed the windows/run-tests branch from 5bd3c31 to f2237ee Compare January 26, 2023 13:02
@cderv cderv marked this pull request as ready for review February 15, 2023 23:05
@cderv
Copy link
Collaborator Author

cderv commented Feb 15, 2023

This is now ready for integration. I just launched a last test with the latest change from main.

All tests passes except 2 or 3 I think that I ignore for now until we fixe them. There are quite a few changes for local environment so it is worth reading and checking before we merge. A README is available to explain.

This PR

  • Revisit how we set up our testing environment locally (for R Python Julia) - Currently it is with locked state we'll update on the go. Challenge was to have something working for multi OS seemlessly; Using pipenv for Python helps with that
  • Adapte run-tests.sh and add a run-tests.ps1
  • Adapt GHA workflow to run on windows
  • Fix some tests
  • Fix some bugs in code (and other fix were done in external PR)
  • If anyone is available tomorrow for a quick demo and presentation, and to test the new local configuration for test environment that would be cool.

Hopefully the change will work for everyone local environment.

Limitation and what to do next

Currently we rely on testing our code with locked state of packages for Julia, R and Python. This is using tools that will rely on lockfile to recreate a fix state.

This means that we'll need to update the different lockfile on regular basis - I should add a note in the README probably

Making it possible to test with latest version of all packages will be next in line among test environment improvment.

@cderv cderv requested a review from dragonstyle February 15, 2023 23:16
@cderv
Copy link
Collaborator Author

cderv commented Feb 15, 2023

@dragonstyle putting you as reviewer as you seemed to be available and interested to test all this locally together.

# Ensure that tinytex is installed
quarto install tinytex --no-prompt
# Activating python virtualenv
if [[ -z $VIRTUAL_ENV ]]
Copy link
Collaborator

Choose a reason for hiding this comment

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

if [[ -z $FORCE_NO_PIPENV ]]
then
  OLD_VIRTUAL_ENV=$VIRTUAL_ENV
  if [[ -n $VIRTUAL_ENV ]]
  then
    deactivate
  end
  echo "> Activating virtualenv for Python tests"
  source "$(pipenv --venv)/bin/activate"
  quarto_venv_activated="true"
end
# ...
# at the end
# this is probably not correct if we were not in a venv but you get the idea.
if [[ -n $OLD_VIRTUAL_ENV ]]
then
  deactivate
  source $OLD_VIRTUAL_ENV/bin/activate
end

Copy link
Collaborator Author

@cderv cderv Feb 16, 2023

Choose a reason for hiding this comment

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

@cscheid

So I did another approach as deactivating was not robust. For example I am using pyenv on Linux to manage my python and virtualenv and deactivate was not existing.

So I leveraged some pipenv feature. Here is how it works

  • pipenv will always create a virtual environment even when run in another virtuaenv. This means by default it will activate the new virtualenv. This environment will be located in tests/.venv
  • If I detect that $VIRTUAL_ENV is set though before activating with pipenv, I remember this, and at the end, I run source $OLD_VIRTUAL_ENV/bin/activate to reactivate the initial virtualenvironment.
  • To skip the default behavior QUARTO_TESTS_FORCE_NO_PIPENV can be set to any value. In that case, tests are running in the current python environment with no change.

I believe this work well and answer our discussion.

@cderv cderv marked this pull request as draft February 16, 2023 17:22
@cderv
Copy link
Collaborator Author

cderv commented Feb 16, 2023

Thanks @cscheid and @dragonstyle - I'll still have some last adjustments as discussed today with you both, and this will be ready to merge tomorrow ! 🎉

So cool !

…virtualenv shell

This relies on pipenv features to ignore virtualenv and create a new one.

Add a mechanism to skip using pipenv
@cderv cderv marked this pull request as ready for review February 17, 2023 00:06
@cderv
Copy link
Collaborator Author

cderv commented Feb 17, 2023

I did the modification for the virtual environment, and added some information to the documentation. Ready now to merge !

@cscheid
Copy link
Collaborator

cscheid commented Feb 17, 2023

Feel free to merge anytime!

@cderv cderv merged commit 38b6ab0 into main Feb 17, 2023
@cderv cderv deleted the windows/run-tests branch February 17, 2023 11:28
@cderv cderv linked an issue Mar 7, 2023 that may be closed by this pull request
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.

cannot locate deno: issue with configure.cmd

4 participants