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

Top Secret could use another test #1315

Closed
krstopro opened this issue Apr 21, 2023 · 2 comments · Fixed by #1317
Closed

Top Secret could use another test #1315

krstopro opened this issue Apr 21, 2023 · 2 comments · Fixed by #1317

Comments

@krstopro
Copy link
Contributor

krstopro commented Apr 21, 2023

Exercise Top Secret, decode_secret_message_part/2 function. I believe there should be another test when the AST (the first argument of the fucntion) is formed from the code consisting of only primitive types such as 12 or :ok.
For example, if ast = TopSecret.to_ast("12") or ast = TopSecret.to_ast(":ok"), then TopSecret.decode_secret_message_part(ast, acc) should return {ast, acc}. However, there is no test for that; all the tests assume that the AST is formed from the code containing a function. One could pass all the tests even though the function raises an ArgumentError for such an argument. This test could as well help the solver with task the Task 5 of the exercise as some nodes simply contain only primitive types; took me quite some time to notice that one should check if the ast in the function is actually a tuple.

I am willing to do this myself, but if I am correct one should first open an issue here.

@angelikatyborska
Copy link
Contributor

Hi! Thank you for the suggestion - it's a very good one. I would be very happy to see a PR from you with that improvement 🙂. The new test should be a separate test case, probably added after the existing "returns the AST and accumulator unchanged" test.

@krstopro
Copy link
Contributor Author

Done! I will ask you to review the PR.

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 a pull request may close this issue.

2 participants