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

Move specs from node specs to vhost stats spec #643

Merged
merged 1 commit into from
Apr 12, 2024

Conversation

spuun
Copy link
Member

@spuun spuun commented Mar 20, 2024

WHAT is this pull request doing?

Moving and refactoring specs.

For some reason these moved specs was located in the file for api nodes controller specs. These specs aren't testing the nodes api, they are testing vhost stats.

HOW can this pull request be tested?

Run specs

I doesn't make sense to have these specs in nodes specs. They are
actually testing vhost stats.
@viktorerlingsson
Copy link
Member

viktorerlingsson commented Mar 20, 2024

Maybe we should keep both spec files? /api/nodes also returns metrics that we use on the node page, right? So I think it makes sense to have tests for that as well.

@spuun
Copy link
Member Author

spuun commented Mar 22, 2024

Maybe we should keep both spec files? /api/nodes also returns metrics that we use on the node page, right? So I think it makes sense to have tests for that as well.

True, but I think we can test all that in the one spec that's left. Just add more keys to verify. All "should update metrics" things has nothing to do with the API. It may even be an idea to move API specs to playwright...

I did this because I had an idea to break out stats from LavinMQ::Server to be able to do even more isolated specs and to get smaller classes.

@viktorerlingsson
Copy link
Member

True, but I think we can test all that in the one spec that's left. Just add more keys to verify. All "should update metrics" things has nothing to do with the API.

OK, sounds good! 👍

@spuun spuun merged commit c13e572 into main Apr 12, 2024
19 checks passed
@spuun spuun deleted the reorganize-vhost-stats-specs branch April 12, 2024 05:49
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.

3 participants