Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Jameswtruher/travisdaily2#2842

Merged
TravisEz13 merged 3 commits into
PowerShell:masterPowerShell/PowerShell:masterfrom
JamesWTruher:jameswtruher/travisdaily2JamesWTruher/PowerShell-1:jameswtruher/travisdaily2Copy head branch name to clipboard
Dec 6, 2016
Merged

Jameswtruher/travisdaily2#2842
TravisEz13 merged 3 commits into
PowerShell:masterPowerShell/PowerShell:masterfrom
JamesWTruher:jameswtruher/travisdaily2JamesWTruher/PowerShell-1:jameswtruher/travisdaily2Copy head branch name to clipboard

Conversation

@JamesWTruher

Copy link
Copy Markdown
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

lzybkr commented Dec 5, 2016

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

lzybkr commented Dec 5, 2016

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
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

lzybkr commented Dec 5, 2016

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

iSazonov commented Dec 6, 2016

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
Collaborator Author

can we please merge this now?

@lzybkr

lzybkr commented Dec 6, 2016

Copy link
Copy Markdown
Contributor

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.

# (when calling get-help -online) might not matched the one in the csv file.

BeforeAll {
$SavedProgressPreference = $ProgressPreference

Copy link
Copy Markdown
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

@JamesWTruher JamesWTruher Dec 6, 2016

Copy link
Copy Markdown
Collaborator Author

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


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

Copy link
Copy Markdown
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

Morty Proxy This is a proxified and sanitized view of the page, visit original site.