Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
FI-2748: Add TestKit to template #561
FI-2748: Add TestKit to template #561
Changes from all commits
203ffdf
73e8e99
9d85bbe
95aa033
ed03bda
67fc709
84a82e0
1e23e69
868b193
aec0949
0bf87f1
a95b61b
bbf5bca
f5d0a7a
e8619c7
e630321
ab89a95
cd5e7a1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not wild about
Metadata
as the class name. MaybeTestKitMetadata
would be better?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the
Metadata
, it makes the global nameSomethingTestKit::Metadata
which makes sense since the RubyGem itself is the Test Kit,SomethingTestKit
is the namespace, andMetadata
is really just metadata and maybe config.If anything I would rename/alias
Inferno::TestKit
toInferno::TestKitMetadata
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move the
class Metadata
(or whatever we call it) into lib/%library_name%.rb.tt (i.e: lib/my_test_kit.rb)? And then the content currently in lib/my_test_kit.rb should be moved into lib/my_test_kit/my_test_kit_suite.rb. Then people can copy-paste the suite file to kick start Test Kits that house multiple Test Suites like PDex.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do think we should move the current contents into a separate suite file, but I don't think we should move the metadata there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so
lib/my_test_kit.rb
will only have a couple of includes in it?I don't care that much one way or another, but Shaumik's suggestion sounds pretty reasonable to me. This way
lib/my_test_kit.rb
contains metadata about the test kit in a very obvious file (the 'most important' file), and we have fewer files to deal with in the main gem directory? I can see if you wanted to make it so you can load just the Test Kit metadata without loading everything "within" the test kit too. But is there a use case for that? Unfortunately we can't reference this thing in our .gemspec, which means it can't be used like version.rb is used and why its great to have that in its own file.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, my goal was to be able to load just the test kits without loading the suites, but I guess the platform will need the suites anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should force a standard here. I'm ok not for now though i suppose just to get through this first iteration.