Skip to content

feat: BED-5468 - added a graph fixture loader for graph json files #1335

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Apr 11, 2025
Merged

feat: BED-5468 - added a graph fixture loader for graph json files #1335

merged 7 commits into from
Apr 11, 2025

Conversation

sircodemane
Copy link
Contributor

Description

This adds a function for importing a graph defined in JSON

Motivation and Context

This PR addresses: BED-5468

Why is this change required? What problem does it solve?

Types of changes

  • New feature (non-breaking change which adds functionality)

Checklist:

@sircodemane sircodemane self-assigned this Apr 9, 2025
@sircodemane sircodemane added enhancement New feature or request api A pull request containing changes affecting the API code. tooling This updates developer tooling labels Apr 9, 2025
@sircodemane sircodemane requested a review from superlinkx April 9, 2025 14:42
@superlinkx
Copy link
Contributor

Please run just prepare-for-codereview

superlinkx
superlinkx requested changes Apr 9, 2025
View reviewed changes
Copy link
Contributor

@superlinkx superlinkx left a comment

Choose a reason for hiding this comment

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

In addition to the code bits, has this been tested in any way to make sure it works?

@sircodemane
Copy link
Contributor Author

sircodemane commented Apr 9, 2025 •
edited
Loading

Please run just prepare-for-codereview

@superlinkx i have become the thing i sought to fix :ded:

@sircodemane
Copy link
Contributor Author

In addition to the code bits, has this been tested in any way to make sure it works?

@superlinkx the original PoC served as a manual function test. Since this is test tooling, tests would be contrived compared to use case testing. That was why I was asking yesterday if I should convert a couple harness tests. The failure points in the code are fairly limited since there's not really any business logic.

@superlinkx
Copy link
Contributor

Having a sample test or even just running this code in some way would be sufficient, I'd just like confirmation that the code has been run in some fashion before we merge, which I'm fairly sure I brought up during that discussion. Doesn't need official tests, just a confirmed code run for now

superlinkx
superlinkx reviewed Apr 9, 2025
View reviewed changes
@sircodemane
feat: BED-5468 - updating graph fixture loader and completeness harness
428fb54 
- Removed the manually coded Completeness harness
- Updated the graph json and svg for the Completeness harness
- Modified the tests that use the manually coded Completeness harness to use fixture loader instead
- Added a property processor with one supported timestamp function `NOW()`
- Fixed a duplicate json field in the local configs
@sircodemane
chore: BED-5468 - fixed error from removed harness and incorrect nil …
f510b60 
…check
@sircodemane sircodemane requested a review from superlinkx April 10, 2025 15:56
superlinkx
superlinkx requested changes Apr 10, 2025
View reviewed changes
Copy link
Contributor

@superlinkx superlinkx left a comment

Choose a reason for hiding this comment

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

Overall looks good, we need to make sure to use the neo4j driver though

mistahj67
mistahj67 reviewed Apr 11, 2025
View reviewed changes
@sircodemane
chore: BED-5468 - reverted inaccurate graph driver change and clarifi…
40e771a 
…ed some loader doc text
@sircodemane
chore: BED-5468 - refactoring graph fixture loader
921137e 
- reworked the semantics of loading fixtures from files and write fixtures to the graph
- improved the completeness tests
superlinkx
superlinkx approved these changes Apr 11, 2025
View reviewed changes
Copy link
Contributor

@superlinkx superlinkx left a comment

Choose a reason for hiding this comment

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

LGTM

@sircodemane sircodemane merged commit 5162db2 into main Apr 11, 2025
8 checks passed
@sircodemane sircodemane deleted the bed-5468-graph-fixture-tooling branch April 11, 2025 18:31
@github-actions github-actions bot locked and limited conversation to collaborators Apr 11, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mistahj67 mistahj67 mistahj67 left review comments

@superlinkx superlinkx superlinkx approved these changes

Assignees

@sircodemane sircodemane

Labels
api A pull request containing changes affecting the API code. enhancement New feature or request tooling This updates developer tooling
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sircodemane @superlinkx @mistahj67