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

Add additional asserts to help investigate test_win_pkg failure #49809

Merged
merged 10 commits into from Oct 13, 2018

Conversation

Projects
None yet
4 participants
@Ch3LL
Copy link
Contributor

commented Sep 27, 2018

What does this PR do?

Currently the test_win_pkg module integration tests is failing sometimes on windows. I cannot replicate this failure so I'm adding additional asserts and running the refresh_db one more time if it does initially fail.

@salt-jenkins salt-jenkins requested a review from saltstack/team-windows Sep 27, 2018

@Ch3LL Ch3LL requested a review from dwoz Sep 27, 2018

@dwoz

dwoz approved these changes Sep 27, 2018

@@ -33,8 +34,24 @@ def test_adding_removing_pkg_sls(self):
Test add and removing a new pkg sls
in the windows software repository
'''
def _check_pkg(pkgs, exists=True):
self.run_function('pkg.refresh_db')
def _check_pkg(pkgs, exists=True, check_refresh=None):

This comment has been minimized.

Copy link
@cachedout

cachedout Sep 30, 2018

Collaborator

Should check_refresh be a boolean by default?

This comment has been minimized.

Copy link
@Ch3LL

Ch3LL Oct 4, 2018

Author Contributor

I could change it to boolean but we are passing in check_refresh=2, when using this so i kept it at None. Is it better practice to keep it a boolean even though we are not passing True/False into the kwarg.

self.assertEqual(0, refresh['failed'],
msg='failed returned {0}. Expected return: 0'.format(refresh['failed']))
self.assertEqual(check_refresh, refresh['total'],
msg='total returned {0}. Expected return {1}'.format(check_refresh, refresh['total']))

This comment has been minimized.

Copy link
@damon-atkins

damon-atkins Sep 30, 2018

Member

msg='total returned {0}. Expected return {1}'.format(refresh['total']), check_refresh) ??

self.assertEqual(check_refresh, refresh['total'],
msg='total returned {0}. Expected return {1}'.format(check_refresh, refresh['total']))
self.assertEqual(check_refresh, refresh['success'],
msg='success returned {0}. Expected return {1}'.format(check_refresh, refresh['success']))

This comment has been minimized.

Copy link
@damon-atkins

damon-atkins Sep 30, 2018

Member

msg='success returned {0}. Expected return {1}'.format(refresh['success']), check_refresh)

if count == 1:
raise AssertionError(err)
count = count -1
refresh = self.run_function('pkg.refresh_db')
repo_data = self.run_function('pkg.get_repo_data', timeout=300)

This comment has been minimized.

Copy link
@damon-atkins

damon-atkins Sep 30, 2018

Member

timeout=30 it just opens an existing file and reads it into ram. 30 seconds is heaps

This comment has been minimized.

Copy link
@Ch3LL

Ch3LL Oct 4, 2018

Author Contributor

looks like that change was made intentionally to also try to stabilize this flaky test so I dont want to remove it quit yet.

This comment has been minimized.

Copy link
@damon-atkins

damon-atkins Oct 8, 2018

Member

You could also call pkg.refresh_db with verbose=True and failhard=False
The data structure return will be this even if it fails (with data not like the example below).

{
    "local": {
        "total": 0,
        "success": 0,
        "success_list": {},
        "failed_list": {},
        "failed": 0
    }
}

With out failhard=False, on bad package definition it will raise an error which returns text and not a data structure like above.

Ch3LL added some commits Sep 27, 2018

@Ch3LL Ch3LL force-pushed the Ch3LL:win_pkg_flaky branch from 9bcf5d0 to 9dce5cc Oct 4, 2018

@dwoz

This comment has been minimized.

Copy link
Contributor

commented Oct 4, 2018

@Ch3LL I'vd made some additions to this: Ch3LL#4

Ch3LL and others added some commits Oct 5, 2018

Merge pull request #5 from dwoz/win_pkg_flaky
Log results of cache_dir for now
Merge pull request #6 from dwoz/win_pkg_flaky
Try running fsync to avoid fs caching race condition
path=repo_details.winrepo_source_dir,
saltenv=saltenv,
include_pat='*.sls',
exclude_pat=r'E@\/\..*?\/' # Exclude all hidden directories (.git)
)
log.debug("refresh_db - Return from cache_dir %s", repr(ret))

This comment has been minimized.

Copy link
@damon-atkins

damon-atkins Oct 8, 2018

Member

Not sure why this is needed, it already spills out heaps of logs in debug. i.e. every file which is pulled down and every file skipped.

@damon-atkins

This comment has been minimized.

Copy link
Member

commented Oct 8, 2018

Just looking at the windows test and there are hundreds of these lines 00:39:41 unix2dos: converting file
Which is not how it works when its running for real, being installed normally. i.e. all SLS files pulled from the master will be LF only, which is how it should be. Unix2Dos should not be used at all.

@dwoz dwoz referenced this pull request Oct 8, 2018

Closed

[WIP] debug win_pkg_test #49944

dwoz and others added some commits Oct 11, 2018

Fix win_pkg test
Bugfix in file_list cache. The file_list cache age could be a negative
number meaning the file_list cache can get used in tests even though the
cache timeout is set to 0 for test suite runs.
@dwoz

This comment has been minimized.

Copy link
Contributor

commented Oct 12, 2018

@damon-atkins We added the unix2dos conversion a month or two ago. This was done because, by default, git will perform newline translations and git clones on Windows will have CRLF line endings. By running unix2dos the tests run with the same line endings as if someone runs the tests from a git clone on windows.

Due to the fact that git can be configured to perform newline translations (or not), using unix2dos is not ideal. Since you could perform a git clone without newline translations and then see some failures in the test suite because of that.

In addition, as you have pointed out, using unix2dos does not cover the case where you have a linux master with windows minion. It means the test suite primarily testing a Windows master with a Windows minion.

I think long term we want to enumerate all possible line ending cases and test all of them. We're just not there yet. A couple other things to think about is the fact that YAML performs line ending translation so that values parsed by YAML always have LF line endings even if the source file has CRLF line endings. JSON on the other hand should always have LF line endings source files.

Hopefully this explains why we are using unix2dos for now and the direction we are heading.

@damon-atkins

This comment has been minimized.

Copy link
Member

commented Oct 13, 2018

Thanks for the response. And assume pip installs would have the original line endings as well from the source.

@dwoz dwoz merged commit fef7669 into saltstack:2018.3 Oct 13, 2018

3 of 10 checks passed

continuous-integration/jenkins/pr-merge This commit cannot be built
Details
jenkins/pr/py2-centos-7 The py2-centos-7 job has failed
Details
jenkins/pr/py2-ubuntu-1604 The py2-ubuntu-1604 job has failed
Details
jenkins/pr/py2-windows-2016 The py2-windows-2016 job has failed
Details
jenkins/pr/py3-centos-7 The py3-centos-7 job has failed
Details
jenkins/pr/py3-ubuntu-1604 The py3-ubuntu-1604 job has failed
Details
jenkins/pr/py3-windows-2016 The py3-windows-2016 job has failed
Details
WIP ready for review
Details
jenkins/pr/docs The docs job has passed
Details
jenkins/pr/lint The lint job has passed
Details
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.