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

Changes to address failing tests in appveyor #34

Closed
wants to merge 19 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@dmoody256
Contributor

dmoody256 commented Dec 23, 2017

Original text lost from flawed tigris bug migration

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Dec 23, 2017

Coverage Status

Coverage remained the same at 59.798% when pulling ffda18f on dmoody256:AppveyorCIClangTests into b5ca6fc on SCons:master.

coveralls commented Dec 23, 2017

Coverage Status

Coverage remained the same at 59.798% when pulling ffda18f on dmoody256:AppveyorCIClangTests into b5ca6fc on SCons:master.

@dmoody256 dmoody256 changed the title from [DEPENDS ON PR 33] Update CLang tests for windows to [DEPENDS ON PR 33] Update appveyor tests Dec 24, 2017

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Dec 24, 2017

Coverage Status

Coverage decreased (-26.7%) to 33.115% when pulling 893c0d8 on dmoody256:AppveyorCIClangTests into b5ca6fc on SCons:master.

coveralls commented Dec 24, 2017

Coverage Status

Coverage decreased (-26.7%) to 33.115% when pulling 893c0d8 on dmoody256:AppveyorCIClangTests into b5ca6fc on SCons:master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Dec 24, 2017

Coverage Status

Coverage remained the same at 59.798% when pulling 893c0d8 on dmoody256:AppveyorCIClangTests into b5ca6fc on SCons:master.

coveralls commented Dec 24, 2017

Coverage Status

Coverage remained the same at 59.798% when pulling 893c0d8 on dmoody256:AppveyorCIClangTests into b5ca6fc on SCons:master.

@dmoody256 dmoody256 changed the title from [DEPENDS ON PR 33] Update appveyor tests to [DEPENDS ON PR 33][WIP] Update appveyor tests Dec 27, 2017

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Dec 27, 2017

Coverage Status

Coverage decreased (-25.8%) to 33.973% when pulling d176632 on dmoody256:AppveyorCIClangTests into b5ca6fc on SCons:master.

coveralls commented Dec 27, 2017

Coverage Status

Coverage decreased (-25.8%) to 33.973% when pulling d176632 on dmoody256:AppveyorCIClangTests into b5ca6fc on SCons:master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Dec 27, 2017

Coverage Status

Coverage increased (+0.04%) to 59.838% when pulling d176632 on dmoody256:AppveyorCIClangTests into b5ca6fc on SCons:master.

coveralls commented Dec 27, 2017

Coverage Status

Coverage increased (+0.04%) to 59.838% when pulling d176632 on dmoody256:AppveyorCIClangTests into b5ca6fc on SCons:master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Dec 28, 2017

Coverage Status

Coverage increased (+0.04%) to 59.838% when pulling 61d3cec on dmoody256:AppveyorCIClangTests into b5ca6fc on SCons:master.

coveralls commented Dec 28, 2017

Coverage Status

Coverage increased (+0.04%) to 59.838% when pulling 61d3cec on dmoody256:AppveyorCIClangTests into b5ca6fc on SCons:master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Dec 28, 2017

Coverage Status

Coverage increased (+0.04%) to 59.838% when pulling 61d3cec on dmoody256:AppveyorCIClangTests into b5ca6fc on SCons:master.

coveralls commented Dec 28, 2017

Coverage Status

Coverage increased (+0.04%) to 59.838% when pulling 61d3cec on dmoody256:AppveyorCIClangTests into b5ca6fc on SCons:master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Dec 28, 2017

Coverage Status

Coverage increased (+0.04%) to 59.838% when pulling fa0583a on dmoody256:AppveyorCIClangTests into b5ca6fc on SCons:master.

coveralls commented Dec 28, 2017

Coverage Status

Coverage increased (+0.04%) to 59.838% when pulling fa0583a on dmoody256:AppveyorCIClangTests into b5ca6fc on SCons:master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Dec 28, 2017

Coverage Status

Coverage increased (+0.04%) to 59.838% when pulling fa0583a on dmoody256:AppveyorCIClangTests into b5ca6fc on SCons:master.

coveralls commented Dec 28, 2017

Coverage Status

Coverage increased (+0.04%) to 59.838% when pulling fa0583a on dmoody256:AppveyorCIClangTests into b5ca6fc on SCons:master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Dec 28, 2017

Coverage Status

Coverage increased (+0.03%) to 59.833% when pulling fa0583a on dmoody256:AppveyorCIClangTests into b5ca6fc on SCons:master.

coveralls commented Dec 28, 2017

Coverage Status

Coverage increased (+0.03%) to 59.833% when pulling fa0583a on dmoody256:AppveyorCIClangTests into b5ca6fc on SCons:master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Dec 28, 2017

Coverage Status

Coverage increased (+0.04%) to 59.838% when pulling fa0583a on dmoody256:AppveyorCIClangTests into b5ca6fc on SCons:master.

coveralls commented Dec 28, 2017

Coverage Status

Coverage increased (+0.04%) to 59.838% when pulling fa0583a on dmoody256:AppveyorCIClangTests into b5ca6fc on SCons:master.

@dmoody256 dmoody256 changed the title from [DEPENDS ON PR 33][WIP] Update appveyor tests to [DEPENDS ON PR 33] Update appveyor tests Dec 28, 2017

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Dec 28, 2017

Coverage Status

Coverage increased (+0.04%) to 59.838% when pulling 9dde17b on dmoody256:AppveyorCIClangTests into b5ca6fc on SCons:master.

coveralls commented Dec 28, 2017

Coverage Status

Coverage increased (+0.04%) to 59.838% when pulling 9dde17b on dmoody256:AppveyorCIClangTests into b5ca6fc on SCons:master.

libraryname = 'foo.dll'
# add the environment, otherwise the environment will consist of only vcvarsall.bat variables
# and not clang
env_str = "import os\nenv = Environment(tools=['clang', 'link'], ENV = os.environ)"

This comment has been minimized.

@bdbaddog

bdbaddog Dec 31, 2017

Contributor

Why do this only for win32?

@bdbaddog

bdbaddog Dec 31, 2017

Contributor

Why do this only for win32?

This comment has been minimized.

@dmoody256

dmoody256 Dec 31, 2017

Contributor

The Environment on windows was created without the original environment that the runtest.py was called from. Not sure if that is an appveyor specific issue or something weird with the msvs tool running vcvarsall.bat, but the mingw tools for linking were missing from the path causing a build failure on the link step during the test.

Adding the os.environ here retained the original environment and path that runtest.py was run from.

@dmoody256

dmoody256 Dec 31, 2017

Contributor

The Environment on windows was created without the original environment that the runtest.py was called from. Not sure if that is an appveyor specific issue or something weird with the msvs tool running vcvarsall.bat, but the mingw tools for linking were missing from the path causing a build failure on the link step during the test.

Adding the os.environ here retained the original environment and path that runtest.py was run from.

@bdbaddog

This comment has been minimized.

Show comment
Hide comment
@bdbaddog

bdbaddog Dec 31, 2017

Contributor

Your changes will disable and/or break tests which do complete successfully on our windows buildbot workers and on properly configured machines.

See: http://buildbot.scons.org/#/builders/8/builds/19

Plus I really don't like huge pull requests which affect many different tests. Ideally you'd address the comments and make sure that the changes aren't disabling tests which work outside the appveyor environment.

The goal is not that only the tests which function in the appveyor environment run, but rather that tests which can pass will run in any environment which can run them and get skipped in those that cannot.

Contributor

bdbaddog commented Dec 31, 2017

Your changes will disable and/or break tests which do complete successfully on our windows buildbot workers and on properly configured machines.

See: http://buildbot.scons.org/#/builders/8/builds/19

Plus I really don't like huge pull requests which affect many different tests. Ideally you'd address the comments and make sure that the changes aren't disabling tests which work outside the appveyor environment.

The goal is not that only the tests which function in the appveyor environment run, but rather that tests which can pass will run in any environment which can run them and get skipped in those that cannot.

@dmoody256

This comment has been minimized.

Show comment
Hide comment
@dmoody256

dmoody256 Dec 31, 2017

Contributor

Your changes will disable and/or break tests which do complete successfully on our windows buildbot workers and on properly configured machines.

See: http://buildbot.scons.org/#/builders/8/builds/19

None of the failed test in the buildbot you linked were modified in this PR, and I dont see any underlying files in this PR which could have affected those tests on buildbot.

Plus I really don't like huge pull requests which affect many different tests. Ideally you'd address the comments and make sure that the changes aren't disabling tests which work outside the appveyor environment.

The goal is not that only the tests which function in the appveyor environment run, but rather that tests which can pass will run in any environment which can run them and get skipped in those that cannot.

I addressed your review comments, if there are certain changes in this PR you feel are changing the tests to disable or break them in other environments, we could check one of the appveyor environment variables and skip the test in those cases for appveyor so as to leave the test the same on other environments, but still be able to get passing results from appveyor.

Contributor

dmoody256 commented Dec 31, 2017

Your changes will disable and/or break tests which do complete successfully on our windows buildbot workers and on properly configured machines.

See: http://buildbot.scons.org/#/builders/8/builds/19

None of the failed test in the buildbot you linked were modified in this PR, and I dont see any underlying files in this PR which could have affected those tests on buildbot.

Plus I really don't like huge pull requests which affect many different tests. Ideally you'd address the comments and make sure that the changes aren't disabling tests which work outside the appveyor environment.

The goal is not that only the tests which function in the appveyor environment run, but rather that tests which can pass will run in any environment which can run them and get skipped in those that cannot.

I addressed your review comments, if there are certain changes in this PR you feel are changing the tests to disable or break them in other environments, we could check one of the appveyor environment variables and skip the test in those cases for appveyor so as to leave the test the same on other environments, but still be able to get passing results from appveyor.

@dmoody256 dmoody256 changed the title from [DEPENDS ON PR 33] Update appveyor tests to Update appveyor tests Dec 31, 2017

dmoody256 added some commits Dec 31, 2017

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jan 1, 2018

Coverage Status

Coverage remained the same at 59.838% when pulling 5d837bd on dmoody256:AppveyorCIClangTests into 91d1191 on SCons:master.

coveralls commented Jan 1, 2018

Coverage Status

Coverage remained the same at 59.838% when pulling 5d837bd on dmoody256:AppveyorCIClangTests into 91d1191 on SCons:master.

@bdbaddog bdbaddog changed the title from Update appveyor tests to HOWTO for release process Jan 2, 2018

@bdbaddog bdbaddog closed this Jan 2, 2018

@bdbaddog bdbaddog added the P3 label Jan 2, 2018

@bdbaddog bdbaddog changed the title from HOWTO for release process to Changes to address failing tests in appveyor Jan 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment