Skip to content
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

Tests: Output Content Provider query related functionality #626

Merged
merged 8 commits into from
Jan 20, 2017

Conversation

Raymondd
Copy link
Contributor

Addresses #584.
The request handler tests seem like they may need to be refactored to be properly testable. I created Issue #625 to track these changed since they are going to be significantly different from these query related tests.

@Raymondd Raymondd added this to the MSSQL Preview Release 3 milestone Jan 18, 2017
@Raymondd Raymondd self-assigned this Jan 18, 2017
@msftclas
Copy link

Hi @Raymondd, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!


It looks like you're a Microsoft contributor (Raymond Martin). If you're full-time, we DON'T require a Contribution License Agreement. If you are a vendor, please DO sign the electronic Contribution License Agreement. It will take 2 minutes and there's no faxing! https://cla.microsoft.com.

TTYL, MSBOT;

@coveralls
Copy link

Coverage Status

Coverage increased (+1.8%) to 61.706% when pulling df5159d on tests/outputContentProvider into 104fec7 on dev.

@coveralls
Copy link

Coverage Status

Coverage increased (+1.2%) to 62.077% when pulling df22f51 on tests/outputContentProvider into 87d00b4 on dev.

Copy link
Contributor

@kevcunnane kevcunnane left a comment

Choose a reason for hiding this comment

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

Approving, but consider my comments please. Thanks

@@ -0,0 +1,76 @@
// TODO: rewrite all the outputprovider handle tests (old ones kept for reference)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd remove this test class if it's no longer relevant. People can go back in Git history if they need to retrieve at all. Unless there's a reason to preserve?

Copy link
Contributor Author

@Raymondd Raymondd Jan 20, 2017

Choose a reason for hiding this comment

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

I implement this TODO in #636. Should have probably created this file in that one, but I will send #636 in right after this one, so this shouldn't be a problem.


// Check if the results window already exists
// Intentionally created anonymously for testing purposes
public displayResultPane = function(resultsUri: string, paneTitle: string): void {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is kinda goofy. I think using TypeMoq you can just mock a real instance of this class and override this one function, avoiding the need to add in a setDisplayResultPane and declare this anonymously. However it's not critical to fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't do a partial Moq, but I did fix this goofy syntax in the class.

@coveralls
Copy link

Coverage Status

Coverage increased (+1.2%) to 62.106% when pulling 482218f on tests/outputContentProvider into 87d00b4 on dev.

@Raymondd Raymondd merged commit a0df43a into dev Jan 20, 2017
@Raymondd Raymondd deleted the tests/outputContentProvider branch January 20, 2017 22:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants