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

Fixes auto-succeeding pester tests in AboutComparison and AboutStrings #214

Merged
merged 5 commits into from
Jul 2, 2019
Merged

Fixes auto-succeeding pester tests in AboutComparison and AboutStrings #214

merged 5 commits into from
Jul 2, 2019

Conversation

EVWorth
Copy link
Contributor

@EVWorth EVWorth commented Jul 2, 2019

PR Summary

This PR fixes pester tests from auto-succeeding.

This PR Resolves Issues #212 and #213

I tested these changes to make sure they initially fail the pester tests successfully

Context

The solution for the AboutComparison issues came from: #212 (comment)

The solution for the AboutString issues came from: #213 (comment)

Changes

In AboutStrings:

  • I replaced text in $String1 on line 105 that was causing the test to succeed without user input.

In AboutComparison.Koans.ps1

  • I replaced 4 instances of __ with $__, where the pester tests were succeeding without user input, because __ was evaluating to $true which completed the test.

Checklist

  • Pull Request has a meaningful title.
  • Summarised changes.
  • Pull Request is ready to merge & is not WIP.
  • Added tests / only testable interactively.
    • Make sure you add a new test if old tests do not effectively test the code changed.
  • Added documentation / opened issue to track adding documentation at a later date.

It "adds strings together" was passing the pester test without user input. Replaced the contents of $String1 with '__' so the user can solve it.
Auto-passing tests:
It 'is a simple test'
It 'returns $true if either input is $true'
It 'negates a boolean value'
In all three instances, __ evaluates to $true, which causes the tests to pass. 

I replaced __ with $__ in these instances to make sure the tests fail.
@vexx32
Copy link
Owner

vexx32 commented Jul 2, 2019

Looks good! Can we use $____ rather than $__ (four underscores instead of two) just so it's very clear we're not trying to reference $_?

Added two extra underscores to each $__ to make sure no one will confuse $__ for $_
@EVWorth
Copy link
Contributor Author

EVWorth commented Jul 2, 2019

Good point/idea!

Done!

Copy link
Owner

@vexx32 vexx32 left a comment

Choose a reason for hiding this comment

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

Looks much better, thank you! 😄

One last one and this is good to merge!

PSKoans/Koans/Foundations/AboutStrings.Koans.ps1 Outdated Show resolved Hide resolved
Elliot Worth and others added 2 commits July 2, 2019 11:21
Longer blanks! Better blanks!

Co-Authored-By: Joel Sallow (/u/ta11ow) <32407840+vexx32@users.noreply.github.com>
@EVWorth
Copy link
Contributor Author

EVWorth commented Jul 2, 2019

I like the longer blanks. They stand out more which is always a good thing. Do you have a preference on standardizing the number of underscores?

perhaps 4 of each? ____ for int/double, "____" for string, and $____ for Boolean?

I know that the Foundations Koans could use a good freshening up in that regard. sounds like a project... 😄

Copy link
Owner

@vexx32 vexx32 left a comment

Choose a reason for hiding this comment

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

Looks good, thank you! 💖

@vexx32
Copy link
Owner

vexx32 commented Jul 2, 2019

@EVWorth yeah it's changed over time as I realize this might be a bit clearer for various reasons. I'd say.... if we're expecting:

  • Numeric input, go with __
  • Strings, go with '____'
  • Variables, go with $____
  • Command names or strings in command arguments, go with ____ unless the string you expect would normally require quotes to enter it as a command argument.

Of course, with strings it's good to think about whether the string you expect might make use of interpolation and should thus use double quotes instead of single quotes. 🙂

@vexx32 vexx32 merged commit 09f013c into vexx32:master Jul 2, 2019
@EVWorth
Copy link
Contributor Author

EVWorth commented Jul 2, 2019

Ok awesome. I'll use that going forward. I was thinking about the ' ' vs "" thing earlier. I think it makes sense to differentiate between those as well.

@vexx32
Copy link
Owner

vexx32 commented Jul 2, 2019

🤔 Sigh, I need to add a contributing style guide lmao

@EVWorth
Copy link
Contributor Author

EVWorth commented Jul 2, 2019

If you do I'll contribute to it and ask you 50 billion questions you don't want to have to answer ;)

@vexx32
Copy link
Owner

vexx32 commented Jul 2, 2019

I don't mind answering haha, it's all good. Just something I should do. I guess I'll make an issue so I don't forget. :)

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.

None yet

2 participants