Skip to content

ci: run oldtests in Appveyor #7705

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 8 commits into from
Closed

ci: run oldtests in Appveyor #7705

wants to merge 8 commits into from

Conversation

janlazo
Copy link
Contributor

@janlazo janlazo commented Dec 8, 2017 •
edited
Loading

Ref #7412

This patch is only for running legacy tests in src/nvim/testdir. I don't intend on adding vim patches here or fixing any legacy tests.

@janlazo janlazo changed the title ci: run oldtests in Appveyor [RDY] ci: run oldtests in Appveyor Dec 8, 2017
justinmk
justinmk reviewed Dec 8, 2017
View reviewed changes
src/nvim/testdir/Makefile Outdated
test79.out \

else
SCRIPTS ?= \
Copy link
Member

Choose a reason for hiding this comment

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

Since this is a superset of the Windows list, can we append to it? Something like:

ifneq ($(OS),Windows_NT)
    SCRIPTS := $(SCRIPTS) \
      ... \
      ...

Copy link
Contributor Author

@janlazo janlazo Dec 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

If the user wants to run one test only, doesn't that append the default tests?
I want to omit test17 and test32 in Windows by default.

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 missed TESTNUM which overrides SCRIPT. I think we can append to it.

Copy link
Member

@justinmk justinmk Dec 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Oh, right. What about this:

SCRIPTS_DEFAULT = ... 
ifneq ($(OS),Windows_NT)
  SCRIPTS_DEFAULT := $(SCRIPTS_DEFAULT) \
    ...
endif

SCRIPTS ?= $(SCRIPTS_DEFAULT)
``

justinmk
justinmk reviewed Dec 8, 2017
View reviewed changes
src/nvim/testdir/Makefile Outdated
NVIM_PRG ?= ../../../build/bin/nvim.exe
else
NVIM_PRG ?= ../../../build/bin/nvim
endif
NVIM_PRG ?= ../../../build/bin/nvim
Copy link
Member

Choose a reason for hiding this comment

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

don't need this line now.

justinmk
justinmk reviewed Dec 8, 2017
View reviewed changes
ci/build.bat Outdated
@@ -53,6 +53,12 @@ bin\nvim --version || goto :error
:: Functional tests
mingw32-make functionaltest VERBOSE=1 || goto :error

:: Unit tests
Copy link
Member

Choose a reason for hiding this comment

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

not "Unit" but "Old tests" or "Vim tests"

justinmk
justinmk reviewed Dec 8, 2017
View reviewed changes
src/nvim/testdir/Makefile Outdated
test14.out \
test24.out \
test37.out \
test40.out \
Copy link
Member

Choose a reason for hiding this comment

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

Same indentation as before, to minimize the diff.

@marvim marvim added the RDY label Dec 8, 2017
@janlazo janlazo changed the title [RDY] ci: run oldtests in Appveyor ci: run oldtests in Appveyor Dec 8, 2017
justinmk pushed a commit that referenced this pull request Dec 10, 2017
@janlazo @justinmk
ci: run oldtests in Appveyor #7705
4a5bc62
@justinmk
Copy link
Member

Merged, thanks @janlazo .

Note that some tests are failing on AppVeyor (search for FAILED: in the appveyor log), but seems like the exit code is not being propagated (so AppVeyor is green). Maybe it's related to endlocal (but https://ss64.com/nt/endlocal.html says "ENDLOCAL does not reset %errorlevel%"). I merged this anyway because it's a useful start.

test13 was removed in 165ba3e, I removed it from the Makefile also.

I forgot that SCRIPTS (SCRIPTS_DEFAULT) are the Vim "old style" tests. I would suggest not spending any time trying to get those to work on AppVeyor. The "new style" tests (NEW_TESTS in the Makefile) are more important.

@justinmk justinmk closed this Dec 10, 2017
@justinmk justinmk removed the RDY label Dec 10, 2017
@justinmk justinmk mentioned this pull request Dec 10, 2017
Merged
@janlazo janlazo deleted the win_oldtests branch December 10, 2017 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justinmk justinmk justinmk left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@janlazo @justinmk @marvim