Skip to content

Add record field type tests#1192

Merged
Schamper merged 10 commits intofox-it:mainfrom
JSCU-CNI:improvement/test-record-field-types
Jul 17, 2025
Merged

Add record field type tests#1192
Schamper merged 10 commits intofox-it:mainfrom
JSCU-CNI:improvement/test-record-field-types

Conversation

@JSCU-CNI
Copy link
Contributor

This PR adds tests to prevent duplicate field names with incompatible field types as identified and fixed in #1189, #990 and #982.

Copy link
Member

@Schamper Schamper left a comment

Choose a reason for hiding this comment

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

Can you add an example in the docstring for what this actually checks/calls out? It's a bit hard to determine from just the test.

@codspeed-hq
Copy link

codspeed-hq bot commented Jun 24, 2025

CodSpeed Performance Report

Merging #1192 will not alter performance

Comparing JSCU-CNI:improvement/test-record-field-types (62cd2d0) with main (a7ab2b4)

Summary

✅ 5 untouched benchmarks

@JSCU-CNI JSCU-CNI requested a review from Schamper June 25, 2025 14:27
@JSCU-CNI
Copy link
Contributor Author

Can you add an example in the docstring for what this actually checks/calls out? It's a bit hard to determine from just the test.

Added in 2439d34. Please let me know if this makes more sense now.

@JSCU-CNI JSCU-CNI requested a review from Schamper July 14, 2025 11:36
Copy link
Member

@Schamper Schamper left a comment

Choose a reason for hiding this comment

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

Maybe just generalize the [] case, since every flow.record type can be made a list. Tests are failing because of a missing uint16[]

@JSCU-CNI
Copy link
Contributor Author

Fixed in 37adb05.

@JSCU-CNI JSCU-CNI requested a review from Schamper July 14, 2025 13:24
@JSCU-CNI
Copy link
Contributor Author

The changes in #1189 will make the tests in this PR pass, would you like to merge those changes in this PR or to merge that PR first?

@Schamper
Copy link
Member

The changes in #1189 will make the tests in this PR pass, would you like to merge those changes in this PR or to merge that PR first?

I know, I have some pending comments on that. Just marking this as approved already, but waiting to merge of course.

@codecov
Copy link

codecov bot commented Jul 17, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.37%. Comparing base (8911ec6) to head (62cd2d0).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1192   +/-   ##
=======================================
  Coverage   78.37%   78.37%           
=======================================
  Files         366      366           
  Lines       33476    33476           
=======================================
  Hits        26237    26237           
  Misses       7239     7239           
Flag Coverage Δ
unittests 78.37% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Schamper Schamper merged commit 307bc1c into fox-it:main Jul 17, 2025
26 checks passed
@JSCU-CNI JSCU-CNI deleted the improvement/test-record-field-types branch July 18, 2025 08:47
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.

2 participants