Added extra test coverage for validate_dnssec_job #2811
Conversation
|
Pull request testing fails. Probable cause is timing in test method: |
|
Resolved the problem with ENV['nameserver_validation_timeout'] |
| original_timeout = ENV['nameserver_validation_timeout'] | ||
|
|
||
| # Seadista kindel väärtus testiks | ||
| ENV['nameserver_validation_timeout'] = '4' |
There was a problem hiding this comment.
Translate comment to English, because registry is Open Source product.
Also, among the English comments, this one stands out.
There was a problem hiding this comment.
Replaced this comment with more explanatory English comment.
| def test_prepare_validator_configures_dnsruby_resolver_with_correct_parameters | ||
| job = ValidateDnssecJob.new | ||
|
|
||
| # To store original environment variable |
There was a problem hiding this comment.
This comment doesn't really add any meaning — it just explains what the code is doing, which is already obvious. So there are two options: either delete the comment, or rewrite it to explain what the nameserver_validation_timeout value does and how it affects the test.
There was a problem hiding this comment.
Removed the unnecessary comment.
|
|
||
| resolver = job.send(:prepare_validator, "8.8.8.8") | ||
|
|
||
| assert_instance_of Dnsruby::Resolver, resolver |
There was a problem hiding this comment.
Are we testing type safety or functionality here? Please clarify, because otherwise I don't see the point of this check.
There was a problem hiding this comment.
While Ruby is dynamically typed language, so it is meant that code behaves correctly at runtime. It is functional guarantee we can use resolver object in next steps.
| valid_to: 1.year.from_now | ||
| ) | ||
|
|
||
| # Add DNSKEY to domain |
There was a problem hiding this comment.
remove this comment
| job = ValidateDnssecJob.new | ||
| job.define_singleton_method(:logger) { logger } | ||
|
|
||
| # Run the job |
| job.perform(domain_name: domain.name) | ||
|
|
||
| # Verify that the domain was skipped | ||
| assert_match /No related nameservers for this domain/, log_output.string |
There was a problem hiding this comment.
I think you've overcomplicated the test a bit. If you look at the file app/jobs/validate_dnssec_job.rb, the dns key object has a field called validation_datetime, which gets updated when the validation is successful. You could’ve used that for the check.
Of course, if the goal is to verify the logs, then tracking logs makes sense too. But what's really important here is that the job didn’t make any changes to the database record.
We actually need two clear tests:
One test that confirms the table was updated when a nameserver is present.
And this current test should prove that validation was skipped, meaning no changes happened in the table at all.
There was a problem hiding this comment.
Thanks! Rewrote test logic.
| end | ||
|
|
||
| def perform_without_domain_name_executes_else_block | ||
| # Use existing domain fixture |
| # Use existing domain fixture | ||
| domain = domains(:shop) | ||
|
|
||
| # Add DNSKEY if not present |
There was a problem hiding this comment.
you can rewrite this part more like "ruby-style":
domain.dnskeys << @dnskey unless domain.dnskeys.any?
There was a problem hiding this comment.
Thanks for pointing out. Rewrote the code.
|
|
||
| ValidateDnssecJob.perform_now() | ||
|
|
||
| assert true, "Job finished without errors" |
There was a problem hiding this comment.
I’m not quite sure I understand the point of this test. What happens if there’s no nameserver — does it still complete without errors? What exactly is the success criteria here? Just checking that the job runs?
What we really need is to confirm that the job actually did its job — meaning the data we expect to change has actually changed (or hasn’t, depending on the case).
I know the job we’re testing includes exception handling, but those should have their own separate tests. Here, the main check should be whether the expected data changes happened — that's the real sign the job worked correctly.
There was a problem hiding this comment.
Rewrote those two tests a bit clearer.
| ) | ||
| end | ||
|
|
||
| ValidateDnssecJob.perform_now() |
There was a problem hiding this comment.
Remove the parentheses.
Thank you for really valuable feedback! |
No description provided.