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

Writing tests [Rust] #9

Closed
AntoniosBarotsis opened this issue Sep 15, 2022 · 9 comments · Fixed by #25
Closed

Writing tests [Rust] #9

AntoniosBarotsis opened this issue Sep 15, 2022 · 9 comments · Fixed by #25
Assignees
Milestone

Comments

@AntoniosBarotsis
Copy link
Owner

There are currently no tests present in the project. While it is very easy to test manually, automated tests are definitely quite handy.

I'm interested in testing that downloading and parsing (separately) RSS feeds are working properly. There should be a way to define a list of feeds to be used in these tests.

Feel free to make any additional tests!

Adding automated code coverage reports would also be a very much welcome addition.

@Kineolyan
Copy link
Contributor

Hello @AntoniosBarotsis, if that's ok with you, I would be interesting to work on this issue as part of the Hacktoberfest.
I plan to start with the two tasks you mention and see what to do next as I dive into the codebase.

One question I am asking myself for which I did not find an answer in your README or CONTRIBUTING: what is your standard workflow for running this?
I see that you have a main version launching it and an aws flavor.
Though that should not impact my ability to write tests, I am curious.

Cheers

@AntoniosBarotsis
Copy link
Owner Author

AntoniosBarotsis commented Sep 28, 2022

Hello!

Yes I think I do not mention this explicitly but a cargo run will run the code as you'd expect without$^1$ sending an email (or requiring the relevant credentials), if you instead run this in release, then the email would also be sent.

The AWS-Lambda stuff you can ignore as the only thing that changes is some boilerplate around the main function.

1: Halfway through writing this I realized that I actually try to resolve the API key early which I shouldn't do, I'll push a commit fixing that in a few minutes and I'll try to include the basic outline of running the app in the contributing file. I had some of this stuff in the wiki but I'll be sure to make that more obvious as well.

Let me know if you have any other questions :)

EDIT: The commit I mentioned has been pushed.

@AntoniosBarotsis
Copy link
Owner Author

AntoniosBarotsis commented Sep 28, 2022

I made a few tweaks to the README and CONTRIBUTING files as well as the Wiki, let me know if its more clear now!

I will try to restructure the docs at some point because information feels like it's all over the place right now.

@Kineolyan
Copy link
Contributor

Looks clear enough. Thanks for the update.
I will dive in the code tomorrow, as it is time for me to sleep 😄

@AntoniosBarotsis AntoniosBarotsis added this to the v1.0 milestone Oct 10, 2022
@AntoniosBarotsis
Copy link
Owner Author

Hello @Kineolyan! I am wondering if you are still working on some tests (you mentioned something about parsing) or if I should close the issue.

Not in a hurry or anything just wasn't sure 👍

@Kineolyan
Copy link
Contributor

Hello @AntoniosBarotsis, I have started another branch for parsing tests https://github.com/Kineolyan/Rss2Email/tree/add-tests-for-parsing

I already have tests for the parsing of Atom feeds and hope to work on RSS this week. Unlike the previous PR I made, that focused on download, I wanted to cover the whole topic before submitting a PR :)

@AntoniosBarotsis
Copy link
Owner Author

That's totally fine and thanks for yet another contribution! I was just going over some issues and wanted to make sure that this is still open :)

@Kineolyan
Copy link
Contributor

Hello @AntoniosBarotsis,

I have a question about RSS. The spec for RSS 0.91 [1] and RSS 0.92 [2] does not define any date for the entries. There is one date at the level of the channel but representing the update date of the whole file.
Given that your goal is to only retrieve the last posts of blogs, I would say that you do not want to support those specs.
Currently, they are not supported because serde parsing will fail. I can simply write tests specifying this for the record. Or do you prefer I update the parsing logic to support those two formats?

[1] https://www.rssboard.org/rss-0-9-1
[2] https://www.rssboard.org/rss-0-9-2

@AntoniosBarotsis
Copy link
Owner Author

I based my implementation according to RSS 2.0 which does include dates. Most if not all feeds that I have seen are using 2.0.

@AntoniosBarotsis AntoniosBarotsis linked a pull request Oct 13, 2022 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants