chore: Tidy up tests and logging typos #24801
Conversation
Signed-off-by: Adam Harvey <33203301+adamdmharvey@users.noreply.github.com>
Missing ChangesetsThe following package(s) are changed by this PR but do not have a changeset:
See CONTRIBUTING.md for more information about how to add changesets. Changed Packages
|
The metadata field is required by schema, and this was testing if it was optional, passing only because it was misspelled. Signed-off-by: Adam Harvey <33203301+adamdmharvey@users.noreply.github.com>
Signed-off-by: Adam Harvey <33203301+adamdmharvey@users.noreply.github.com>
Signed-off-by: Adam Harvey <33203301+adamdmharvey@users.noreply.github.com>
adamdmharvey
force-pushed
the
adamdmharvey-tidy-up
branch
from
5557a7f
to
cfa0afb
Compare
| it('handles missing metadata gracefully', async () => { | ||
| delete data.medatata; | ||
| await expect(policy.enforce(data)).resolves.toBe(data); | ||
| }); |
There was a problem hiding this comment.
entities require metadata, so they shouldn't handle it being missing gracefully. test only passed because metadata was misspelled?
There was a problem hiding this comment.
alternatively, could change this to throw an error if it's missing, just rewriting the test to reject missing metadata.
There was a problem hiding this comment.
I think it's fine to drop 馃憤
It just throws "cannot read ... of undefined" anyway, so it's not really handling it at all. Metadata is required in the TS type too and it's up to the catalog processing engine to ensure that it exists.
adamdmharvey
changed the title
| it('handles missing metadata gracefully', async () => { | ||
| delete data.medatata; | ||
| await expect(policy.enforce(data)).resolves.toBe(data); | ||
| }); |
There was a problem hiding this comment.
I think it's fine to drop 馃憤
It just throws "cannot read ... of undefined" anyway, so it's not really handling it at all. Metadata is required in the TS type too and it's up to the catalog processing engine to ensure that it exists.
|
Uffizzi Cluster |
Hey, I just made a Pull Request!
馃Ч A tidy up PR to fix a few comments in tests and logging.
Also, the entity validator test was useless. It was testing that the metadata field is optional on entities, which the spec defines it is not:
backstage/packages/catalog-model/src/schema/Entity.schema.json
Line 27 in d31ddb5
The only reason the test was working, was because it was trying to delete a field which never existed, and testing if an object = the same object unmodified. If you fix the field name so the test actually does remove metadata, the test fails. So essentially the test is pointless, and I removed it.
I've intentionally not created a changesets for the two modified packages, as most are test updates and the others are trivial loggingedge cases which I thought weren't worth a release. Happy to have counter opinions.
鉁旓笍 Checklist
Signed-off-byline in the message. (more info)