Cyclone/improvements #42
Conversation
CodeCyclone
commented
Jun 27, 2018
CodeCyclone
commented
- Updated documentation for issue Update Get-PowerBIDataset for user API #40
- Implemented issue Support -Tenant\-TenantId in Connect-PowerBIServiceAccount #39 added -Tenant to Connect-PowerBIServiceAccount
- Fixing piping dataset into Get-PowerBIDatasouce
The parameter sets were not defined enough so there was no ambiguity between them.
CodeCyclone
added
bug
nandemoii
left a comment
nandemoii
left a comment
There was a problem hiding this comment.
If these changes fix bugs, I would expect to see unit tests added.
| ### -Scope | ||
| Indicates scope of the call. Individual returns only datasets assigned to the caller; Organization returns all datasets within a tenant (must be an administrator to initiate). Individual is the default. | ||
| Indicates scope of the call. Individual returns datasets from "My Workspace" unless -Workspace or -WorkspaceId is specified which shows datasets under the workspace assigned to the caller; Organization returns all datasets within a tenant (must be an administrator to initiate). Individual is the default. |
There was a problem hiding this comment.
This is confusingly worded. Is this true to what you had in mind?
Indicates scope of the call. Individual returns datasets from "My Workspace" by default. Using the -Workspace or -WorkspaceId parameter, you can show datasets from any workspace assigned to the caller.
There was a problem hiding this comment.
I'm going to change it to this:
Indicates scope of the call. Individual returns datasets from "My Workspace" by default. With -Workspace or -WorkspaceId, datasets under the workspace assigned to the caller are returned; Organization returns all datasets within a tenant (must be an administrator to initiate). Individual is the default.
* Added dataset non-interactive test * Added interactive piping dataset to get datasource * Adeed non-interactive of getting datasource from dataset object * Added non-interactive test for Connect-PowerBIServiceAccount with -Tenant
| /* | ||
| * Requirement to run test: | ||
| * Need at least one dataset containing a datasource assigned to the user logging into the test. | ||
| * Update the test with the dataset ID before running. |
There was a problem hiding this comment.
You should consider updating this and other tests to act similar to the end-to-end tests for the workspace cmdlets.
Those tests don't require you to update the test with any specific ID because they attempt to locate the first valid workspace using a shared helper method (that calls the get cmdlet). If no valid workspace is found, it returns inconclusive. After this check, it will run the intended cmdlets using the ID or workspace object.
This flow makes it very simple to run the end-to-end tests locally and figure out which scenarios weren't hit.
| public TestPowerBICmdletNoClientInitFactory(bool setProfile, IPowerBIProfile profile = null) : | ||
| base(new TestLoggerFactory(), new ModuleDataStorage(), new TestAuthenticator(), new PowerBISettings()) | ||
| { | ||
| if(setProfile) |
There was a problem hiding this comment.
I think it might be helpful to add a comment explaining the intended test scenarios that this boolean parameter allows you to exercise.
| // Assert | ||
| TestUtilities.AssertNoCmdletErrors(ps); | ||
| Assert.IsNotNull(results); | ||
| Assert.IsTrue(results.Count > 0); |
There was a problem hiding this comment.
Consider using Linq (results.Any()) and possibly asserting inconclusive if no results are found.
…ortant for tests than end-users but it should reduce test errors due to Global Service issues.
