-
-
Notifications
You must be signed in to change notification settings - Fork 817
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
Update tests to be able to run them outside of AppVeyor (part 3) #9426
Changes from 18 commits
4ad3ee8
b8d71c5
338c203
e5acd20
e905510
886e639
b6a4cac
379690e
7b292ac
14e8892
74be105
7926fff
936d776
e7ff850
bc506b1
9b5ae5b
8a712b5
397c044
fa98381
944c9cf
142d16e
ac21c00
ee9a982
cdb9eec
5cfe287
b8d137c
82c6434
893d92a
3e5dbb5
eaa1099
192633f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -63,6 +63,7 @@ Describe "$CommandName Integration Tests" -Tags "IntegrationTests" { | |
AfterAll { | ||
$null = Get-DbaDatabase -SqlInstance $script:instance1 -Database $dbname | Remove-DbaDatabase -Confirm:$false | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe here a single Get-DbaDatabase -database $dbname,$dbname2,$dbname3 is shorter and still effective There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good idea. Will update that. |
||
$null = Get-DbaDatabase -SqlInstance $script:instance1 -Database $dbname2 | Remove-DbaDatabase -Confirm:$false | ||
$null = Get-DbaDatabase -SqlInstance $script:instance1 -Database $dbname3 | Remove-DbaDatabase -Confirm:$false | ||
} | ||
|
||
Context "Get history for all database" { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see a real difference in the beforeall and afterall logic, but if it works I trust you. |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. seeing the code, it seems that the previous logic does the correct thing (adding it back if it has been removed) rather than adding it "no matter what" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is e.g. a thing that a test running in a proper CI that gets reset at each run doesn't care about, but if they were to run locally without a "reset phase" it'd be a problem. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will revert this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the change from 13 to 2 is the only thing that I can't readily say "ok, I get it"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me change that back and see what happens...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests are still green. How is that possible?
Maybe because the test failed with an exception:

So we don't catch those errors, or am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you look at the AppVayor output from the dbatools-buildref-index.json run, you see more failed tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup, it seems that those tests have a structure that is not compatible with pester.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the test works locally on my maschine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
problem here is that something either on the beforeall or afterall raises an error (seemingly the offending one is Start-DbaAgentJob -SqlInstance $script:instance2 -Job 'dbatoolsci_testjob').
I'm trying different variations because here, e.g., it seems that there are a sleuth of other jobs already present due to other tests (and the reason why the test creates 2 jobs but checks for 11, which is strange).
Being "bad-dy" ideally this test should clear out any existing job on the agent, create its own, do the tests, and re-cleanup (optionally).
Approaching with a bit of brain, the test creates two jobs and may only make assumptions on those two (and doing so, if one wants to run it locally, it will try - at least - to preserve already existing jobs).
I fear that starting the job (which is the step failing in the beforeall, and avoiding all the tests to be run) is needed for the very last test, but, still, I'm investigating.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aaaand found. jobstep was failing to be created, because appveyor targets "localhost\SQL2016" but the param
-SubsystemServer
needs the real name (doesn't work with "localhost"). JobStep was failing to be created silently, Start-DbaAgent excepted (because it was a job with no steps), Pester4 doesn't handle well exceptions in the beforeall/afterall blocks.