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

Problem 1.1 - New test helper and tests for problem 1.1 #69

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dsomel21
Copy link

@dsomel21 dsomel21 commented Jul 6, 2020

DISCLAIMER: This is my first time in a while forking and making a PR.

I recently came across this repo and I am willing to contribute the best I can.

This PR aims to solve 2 problems:

  • The ability to craft your algorithm and run it against some test cases

For example, if you open up any of the problems in the repo, like this one, you will accidentally see the solution. There should be a separate place where people can JUST attempt the problems after they have read the problem from the book.

This way, we can still offer a set of test cases which users can test against, while still abiding by Gayle's order that is:

do not contain the actual questions

  • A nice way to see how your solution performed

Screen Shot 2020-07-06 at 7 59 08 PM

The caveats to merging:

This is a small PR but a big feature. This would require:

  • Adding a playground (index.js) across all the problems in all the chapters
  • Rewriting the current console.log tests to instead to have the new test() test
  • Possibly crafting new assertion methods depending on the specifications of the algorithm

Please comment, edit or provide criticism to the PR. But in general, I think the playground is a feature we should be going for!

@dsomel21 dsomel21 changed the title Problem 1.1 - New test helper and tets for problem 1.1 Problem 1.1 - New test helper and tests for problem 1.1 Jul 6, 2020
@profnandaa
Copy link
Collaborator

Looks good so far, let me review it some time later in the week. Thanks for the PR! 🎉

@dsomel21
Copy link
Author

Let me know if there any comments or concerns or any way I can improve!

Copy link
Collaborator

@profnandaa profnandaa left a comment

Choose a reason for hiding this comment

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

Anyway to simplify so that the cost of rewriting is not too high? Something like changing console.log to test only. You can make the test descriptions optional.

@dsomel21
Copy link
Author

Okay, let me rethink and come back. Thank you @profnandaa.

In addition, just so we are on the same page, it's not just as simple as replacing console.log with the test keyword, since test helper takes a callback.

For some of them, it can be very easily translatable... For example:

RefactorLogs

Again, this is mainly just aesthetics and better testing. The console.log and test can coexist, so we don't have to pick and choose just ONE.

Do you want me to go ahead and make the description/title of the test optional?

@profnandaa
Copy link
Collaborator

I was thinking of something like:

test(fn(params), expectedResults);

Then abstract all the nice stuff under test(), perhaps including up to the line number where the test is failing?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants