这是indexloc提供的服务,不要输入任何密码
Skip to content
This repository was archived by the owner on Feb 13, 2025. It is now read-only.

Conversation

@svagner
Copy link
Contributor

@svagner svagner commented Mar 3, 2020

Please follow the guide below

Description

Fix test TestNewManager
Use only full hostname for tests (see os.Hostname())

Fixes #2460

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How has this been tested?

  • TestNewManager

Checklist:

  • This contribution follows the project's code of conduct
  • This contribution follows the project's contributing guidelines
  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@svagner svagner force-pushed the fix_TestNewManager branch from 1395ed1 to 6eb3cf5 Compare March 3, 2020 16:45
muffix
muffix previously requested changes Mar 4, 2020
Copy link
Member

@muffix muffix left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution. 👍 I think we need to clarify the comment a little bit and I've got a question about the proposed fix.

@tdinucci
Copy link
Member

tdinucci commented Mar 4, 2020

Yes, thanks for the PR 👍

I think we probably need two tests actually:

  • One where preserveFullHostName == true
  • One where preserveFullHostName == false but this test must then split the copy of expectedHostname in the test on the first . (which the current test isn't doing).

If you would like to update your PR to cover both of these scenarios that would be great. Otherwise I'll be happy to do this.

Copy link
Member

@tdinucci tdinucci left a comment

Choose a reason for hiding this comment

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

This looks good. Thanks very much @svagner 👍

@tdinucci
Copy link
Member

tdinucci commented Mar 8, 2020

Once you bring your branch inline with master we should be able to merge

@svagner
Copy link
Contributor Author

svagner commented Mar 8, 2020

Thanks @tdinucci ! Branch was updated:)

@svagner svagner requested a review from muffix March 8, 2020 22:09
@muffix muffix dismissed their stale review March 9, 2020 12:45

Out of date

@tdinucci tdinucci merged commit 47ed9f3 into bosun-monitor:master Mar 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: TestNewManager failed on some build Linux machines

3 participants