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

Semantic convention generator: Clean up tests using separate input files #63

Closed
Oberon00 opened this issue Sep 8, 2021 · 2 comments
Closed
Labels
cleanup Refactorings, etc, that don't add new features or fix bugs. good first issue Good for newcomers semconv Related to the semantic convention generator.

Comments

@Oberon00
Copy link
Member

Oberon00 commented Sep 8, 2021

Many tests that use separate input files are not written very cleanly. The most important points:

  • The input YAML files contain lots of redundant stuff (copied from real semantic conventions). They should be minimized to only contain what is needed for the test.
  • The Python code of the tests could use some cleanup too similar to what was already done for some markdown tests, see https://github.com/open-telemetry/build-tools/pull/54/files#diff-6efabfcf3a7471d21e99e83685a2d0151cbb1404c52eed86f26a06d81ef17c44 where the test python code was cut down from 544 to 153 lines.
  • Instead of or in addition to the above point, try testing in such a way that we can use Python (triple-quoted) strings as input instead of needing separate files. This is not entirely trivial since file names do play a role (for generating links), but should nonetheless be doable. This would make the tests much, much more readable.

Further ideas, not as important:

  • Asserting on the line in which an exception occurs could be cleaner if we could somehow specify the expected exception in the input (e.g. with a YAML/HTML comment).
@Oberon00 Oberon00 added cleanup Refactorings, etc, that don't add new features or fix bugs. semconv Related to the semantic convention generator. good first issue Good for newcomers labels Sep 8, 2021
@Oberon00
Copy link
Member Author

Oberon00 commented Apr 15, 2022

See also #79 (comment):

These tests are not designed as integration tests, they are targeted feature tests and it makes no sense to drag in a full spec yaml. These tests should instead use a minimal yaml that just uses group foo with attribute bar instead of actual data. See #63.

Having tests that automatically pull in spec code would additionally make sense, but then we don't have e.g. a testMultipleEnum or testRef but instead a testGeneratorIsSpecCompatible (and you'd need to think about how to design the build/change process so that you can also change the output in "incompatible" ways, e.g. temporarily reference a spec PR branch that updates the spec so the semconv table check is in sync again).

@lmolkova
Copy link
Contributor

lmolkova commented Oct 9, 2024

closing this issue - we're sunsetting otel/semconvgen in favor of weaver #322 and won't make any improvements here.

@lmolkova lmolkova closed this as completed Oct 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Refactorings, etc, that don't add new features or fix bugs. good first issue Good for newcomers semconv Related to the semantic convention generator.
Projects
None yet
Development

No branches or pull requests

2 participants