Skip to content

Conversation

@myronmarston
Copy link
Collaborator

The shared example group in gem_spec.rb automatically applies to each ElasticGraph gem. The changes here:

  • Verify that all config classes are defined using ElasticGraph::Config
  • Verify that each config class has a valid JSON schema.
  • Verify that each config class provides a default value for any properties which are not required.
  • Verify that all default and examples values are valid.
  • Verify that every property has a description and examples (which I plan to use in our docs later).
  • Verify that each config class has a unique path.

For now, none of our config classes have been converted to use the new ElasticGraph::Config base class, so I've marked those specs as pending. We'll convert each config class in a later PR.

@CLAassistant
Copy link

CLAassistant commented Aug 12, 2025

CLA assistant check
All committers have signed the CLA.

@myronmarston myronmarston force-pushed the myron/introduce-config-class branch from 6bb5997 to e2bf776 Compare August 12, 2025 01:36
@myronmarston myronmarston force-pushed the myron/enforce-config-class branch 2 times, most recently from 091980e to 7823c11 Compare August 12, 2025 02:13
@myronmarston myronmarston force-pushed the myron/introduce-config-class branch from e2bf776 to faaa62f Compare August 12, 2025 19:53
@myronmarston myronmarston force-pushed the myron/enforce-config-class branch from 7823c11 to 66242c9 Compare August 12, 2025 19:54
The shared example group in `gem_spec.rb` automatically applies to each
ElasticGraph gem. The changes here:

- Verify that all config classes are defined using `ElasticGraph::Config`
- Verify that each config class has a valid JSON schema.
- Verify that each config class provides a `default` value for any properties which are not `required`.
- Verify that all `default` and `examples` values are valid.
- Verify that every property has a `description` and `examples` (which I plan to use in our docs later).
- Verify that each config class has a unique `path`.

For now, none of our config classes have been converted to use the new
`ElasticGraph::Config` base class, so I've marked those specs as pending.
We'll convert each config class in a later PR.
@myronmarston myronmarston force-pushed the myron/enforce-config-class branch from 66242c9 to 3ac7042 Compare August 12, 2025 22:08
@myronmarston myronmarston marked this pull request as ready for review August 12, 2025 23:48
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need its own tests? The logic is complex and feature-packed enough that it almost needs its own tests to validate that it validates correctly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great question. This runs on each CI build and we can see it passing there for confidence that it's not throwing exceptions or failing, but of course that doesn't demonstrate the expected failures when one of the config classes doesn't satisfy the requirements. If this was going to be released as part of ElasticGraph so that it applies to user code, I think that kind of test coverage would be warranted, but for a case like this (where it just runs as part of the test suite here, on the specific class definitions in this code base) I think there's unlikely to be a positive ROI on that investment.

I did get expected failures from each of the expectations in this test logic while working up each PR that migrates a gem to use the new ElasticGraph::Config class, so that gives me a good level of confidence that it works as expected.

Base automatically changed from myron/introduce-config-class to main August 14, 2025 16:50
@myronmarston myronmarston merged commit 51d10a4 into main Aug 14, 2025
11 of 12 checks passed
@myronmarston myronmarston deleted the myron/enforce-config-class branch August 14, 2025 16:50
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.

5 participants