Clean up specs #43
Assignees
Labels
Comments
15 tasks
|
There are 47 failing specs out of 183 total. |
|
Find out which ones we can delete? Which ones need model remediation? which ones will need logic remediation? |
jeremyf
added a commit
that referenced
this issue
Feb 7, 2023
Prior to this commit, we had gems specific for UI testing. As we've worked on this gem, we've removed the UI aspects. The spec times before this change are: ``` Finished in 1 minute 12.18 seconds (files took 7.21 seconds to load) ``` Further, we're not auto-fetching geoname nor lccn nor chronam remote data for this process. The spec times after this change are: ``` Finished in 1 minute 7.31 seconds (files took 6.43 seconds to load) ``` It's not much, but in eliminating some of these gems, we improve the build time as well. Note, we have commented out lots of specs that were broken. It is possible we'll need to reincorporate some of what we removed. Related to: - #43
jeremyf
added a commit
that referenced
this issue
Feb 7, 2023
Prior to this commit, we had gems specific for UI testing. As we've worked on this gem, we've removed the UI aspects. The spec times before this change are: ``` Finished in 1 minute 12.18 seconds (files took 7.21 seconds to load) ``` Further, we're not auto-fetching geoname nor lccn nor chronam remote data for this process. The spec times after this change are: ``` Finished in 1 minute 7.31 seconds (files took 6.43 seconds to load) ``` It's not much, but in eliminating some of these gems, we improve the build time as well. Note, we have commented out lots of specs that were broken. It is possible we'll need to reincorporate some of what we removed. Related to: - #43
jeremyf
added a commit
that referenced
this issue
Feb 7, 2023
Prior to this commit, the `respond_to_missing?` method definition did not have the correct method signature. Which could cause failures elsewhere. Also, as much as we don't want to repeat knowledge regarding implementation details, most references to method_missing and the sibling respond_to_missing duplicate logic; in part to minimize unnecessary calls. (See [Always Define respond_to_missing?][1]) I renamed `name` to `method_name` to be consistent with the parameters of the sibling methods. And last, I made a change in the logic test to short-circuit faster and not reference the file_set if we don't have "agreeable" parameters. References: - #43 [1]: https://thoughtbot.com/blog/always-define-respond-to-missing-when-overriding
jeremyf
added a commit
that referenced
this issue
Feb 7, 2023
Prior to this commit the specs included the following warning:
> WARNING: Using `expect { }.not_to raise_error(SpecificErrorClass)`
> risks false positives, since literally any other error would cause the
> expectation to pass, including those raised by Ruby
> (e.g. NoMethodError, NameError and ArgumentError), meaning the
> code you are intending to test may not even get reached. Instead
> consider using `expect { }.not_to raise_error` or `expect { }.to
> raise_error(DifferentSpecificErrorClass)`.
With this commit, we're removing that warning.
Less warnings, means less chatter.
Related to:
- #43
jeremyf
added a commit
that referenced
this issue
Feb 7, 2023
Prior to this commit, the `respond_to_missing?` method definition did not have the correct method signature. Which could cause failures elsewhere. Also, as much as we don't want to repeat knowledge regarding implementation details, most references to method_missing and the sibling respond_to_missing duplicate logic; in part to minimize unnecessary calls. (See [Always Define respond_to_missing?][1]) I renamed `name` to `method_name` to be consistent with the parameters of the sibling methods. And last, I made a change in the logic test to short-circuit faster and not reference the file_set if we don't have "agreeable" parameters. References: - #43 [1]: https://thoughtbot.com/blog/always-define-respond-to-missing-when-overriding
jeremyf
added a commit
that referenced
this issue
Feb 7, 2023
Prior to this commit the specs included the following warning:
> WARNING: Using `expect { }.not_to raise_error(SpecificErrorClass)`
> risks false positives, since literally any other error would cause the
> expectation to pass, including those raised by Ruby
> (e.g. NoMethodError, NameError and ArgumentError), meaning the
> code you are intending to test may not even get reached. Instead
> consider using `expect { }.not_to raise_error` or `expect { }.to
> raise_error(DifferentSpecificErrorClass)`.
With this commit, we're removing that warning.
Less warnings, means less chatter.
Related to:
- #43
jeremyf
added a commit
that referenced
this issue
Feb 8, 2023
Prior to this commit, the `FakeDerivativeService` leveraged a class instance variable. This is a precarious implementation as we don't reset the variable in-between runs. With this commit, we're shifting this "spy" class to be an instance of the class (thus the tracking values are isolated to each spec run). We do this by having the "spy" conform to the interface of a plugin. Related to: - #43
jeremyf
added a commit
that referenced
this issue
Feb 8, 2023
Prior to this commit, the `FakeDerivativeService` leveraged a class instance variable. This is a precarious implementation as we don't reset the variable in-between runs. With this commit, we're shifting this "spy" class to be an instance of the class (thus the tracking values are isolated to each spec run). We do this by having the "spy" conform to the interface of a plugin. Related to: - #43
jeremyf
added a commit
that referenced
this issue
Feb 8, 2023
Apologies in advance oh intrepid folk. This commit contains quite a bit of conceptual change. The driving purpose is to look at how to reduce the fragility of our generator process while also exposing configurable approaches. First, I am removing generators for each work type and their corresponding indexer. This is precarious, in that as folks add new work types we don't have a mechanism to propagate that. Second, we were previously matching files by work_type, which can work, but we can instead include/prepend logic into those models directly during the `to_prepare` cycle of the IiifPrint engine. Second, instead of having a mixin, I'm looking to create configuration points where you could register a different lineage service. This helps create a more crisp separation of concern. Third, I suspect that some of these "decorations" we do not want to do unless we configure the model for IiifPrint. By removing the generator, we are in greater control of that decision. All of this because I was trying to figure out how to test the prior `ancestor_ids` method. Related to: - #43 - #40
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Summary
Clean up specs.
There's a lot of unused and failing specs due to us taking some but not all pieces of code from the newspaper gem.
Additionally, partially due to the mess, please also review the spec coverage for the new features.
Acceptance Criteria
Screenshots or Video
Testing Instructions
Notes
The text was updated successfully, but these errors were encountered: