10000 Jameswtruher/travisdaily2 by JamesWTruher · Pull Request #2842 · PowerShell/PowerShell · GitHub
[go: up one dir, main page]

Skip to content

Jameswtruher/travisdaily2 #2842

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

Merged

Conversation

JamesWTruher
Copy link
Collaborator

continued attempts to reduce output size so we can get a daily build in travis-ci

this sets progress preference on the help and package management tests to SilentlyContinue to reduce output. This is done for all test execution, including dev and CI

Travis is still complaining that the log file is too big, eliminating progress is
another step to reducing output. It's also not needed for our CI environment.
@lzybkr
Copy link
Contributor
lzybkr commented Dec 5, 2016

Maybe progress should be opt-in instead of opt-out. In other words, disable globally, turn on in the progress tests.

also fix up trailing whitespace
@JamesWTruher
Copy link
Collaborator Author

I thought about disabling progress universally but thought it would be better for each test to control this behavior. My concern is reviewing/rewriting existing tests which may be relying on progress in ways that I don't know.

@lzybkr
Copy link
Contributor
lzybkr commented Dec 5, 2016

Which seems more likely?

  1. Adding new progress tests?
  2. Adding new functionality or tests that write progress?

With code coverage data, I feel comfortable that 1 won't be a big problem. Also, when progress doesn't work correctly, because it's UI, folks tend to notice and complain.

I think 2 will be less hassle in the long run.

@JamesWTruher
Copy link
Collaborator Author

oh, we know we'll be adding a number of progress tests - right now we are only doing the minimum amount of progress tests. As soon as we have better support on non-windows we'll be doing even more. I don't agree that #2 is less hassle

@JamesWTruher
Copy link
Collaborator Author

also, i don't know that this will even solve the problem with Travis-CI, but I have no other way to test it - i would much rather make targeted changes to determine if it will solve the daily build in Travis-CI issue than whole scale changing the way our tests work

@lzybkr
Copy link
Contributor
lzybkr commented Dec 5, 2016

Isn't this really just a bug in PowerShell? Shouldn't we be detecting that we're not running interactively and not show progress?

@JamesWTruher
Copy link
Collaborator Author

That I don't know - I recall that we had difficulties with running (historical) tests because some host APIs would throw when in that mode. This code base is different, of course, but I still would rather avoid tracking down extraneous issues when all I'm trying to do is find the right incantation to get daily builds in Travis-CI to work.
The advice I've gotten so far from the Travis-CI folks is to redirect all output from build/test into a file and not emit it to stdout, but I'm reluctant to try that first, so I am attempting to reduce output in this way.

@PowerShellTeam PowerShellTeam added the Review - Needed The PR is being reviewed label Dec 6, 2016
@iSazonov
Copy link
Collaborator
iSazonov commented Dec 6, 2016

Current solution solved the Travis full log problem. It is very good!

As regards global disable progress bar. On the one hand testing should be performed with the default values as users get them. On the other hand any default parameter changes must not lead to the collapse of the tests, nor to the collapse of the real-world scripts.
Hm... If the progress bar would be turned off by default, then we wouldn't have caught problems with "debris" and performance.

@JamesWTruher
Copy link
Collaborator Author

can we please merge this now?

@lzybkr
Copy link
Contributor
lzybkr commented Dec 6, 2016

Please remove the formatting changes from the code changes, it unnecessarily makes it harder to see the logical code changes.

We have a different PR to remove trailing spaces, so we don't need to do that file by file.

@@ -8,6 +8,8 @@
# (when calling get-help -online) might not matched the one in the csv file.

BeforeAll {
$SavedProgressPreference = $ProgressPreference
Copy link
Member

Choose a reason for hiding this comment

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

I think this is a Pester issues, but this pattern can result in the preference not being set back to the original setting. What should we do about this:

  • File an issue with Pester
  • Use another pattern
  • Live with the issue
  • Some other option

Example Code:

PS C:\Users\tplunk> Describe 'before all after all' {
    BeforeAll{
        Write-Verbose "in BeforeAll" -Verbose
        
    }
    it "it" {
        Start-Sleep -Seconds 2

    }
    AfterAll {
        Write-Verbose "in AfterAll" -Verbose
        
    }
}

Example hitting ctl-c during it:

Describing before all after all
VERBOSE: in BeforeAll
VERBOSE: in it
Note: After all was not run

Example of full run

Describing before all after all
VERBOSE: in BeforeAll
VERBOSE: in it
[+] it 2.13s
VERBOSE: in AfterAll

Copy link
Collaborator Author
@JamesWTruher JamesWTruher Dec 6, 2016

Choose a reason for hiding this comment

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

I'll file an issue with the Pester, this isn't likely to happen in a run (as there's no one to press CRL-C) - This problem is general in pester over all.
I've opened pester/Pester#655

@@ -18,40 +18,44 @@ $InternalSource = 'OneGetTestSource'


Describe "PackageManagement Acceptance Test" -Tags "Feature" {

Copy link
Member

Choose a reason for hiding this comment

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

In the future, please avoid making formatting changes in the same PR as code change per the Coding Conventions.

@TravisEz13 TravisEz13 merged commit 9aa693d into PowerShell:master Dec 6, 2016
@TravisEz13 TravisEz13 added Resolution-Fixed The issue is fixed. and removed Review - Needed The PR is being reviewed labels Dec 6, 2016
@JamesWTruher JamesWTruher deleted the jameswtruher/travisdaily2 branch January 13, 2017 22:30
@iSazonov iSazonov mentioned this pull request Mar 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution-Fixed The issue is fixed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants
0