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

Jump to bottom

fix: skip comments when loading test relationships #335

Merged
merged 1 commit into from Dec 13, 2021
Merged

fix: skip comments when loading test relationships #335

merged 1 commit into from Dec 13, 2021

Conversation

bryanhuhta
Copy link
Contributor

Fixes #329

From a brief test in the playground, comments in the Test Relationships are only of the format // my comment and not /** my comment */. This may need to be fact check though 😁

@github-actions
Copy link

github-actions bot commented Dec 10, 2021 •
edited

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@josephschorr josephschorr self-requested a review December 10, 2021 20:49
@josephschorr
Copy link
Member

From a brief test in the playground, comments in the Test Relationships are only of the format // my comment and not /** my comment */. This may need to be fact check though 😁

Yeah, that's correct :)

@bryanhuhta
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

josephschorr
josephschorr reviewed Dec 10, 2021
View reviewed changes
pkg/validationfile/loader.go
@@ -94,7 +94,7 @@ func PopulateFromFiles(ds datastore.Datastore, filePaths []string) (*FullyParsed
lines := strings.Split(relationships, "\n")
for index, line := range lines {
trimmed := strings.TrimSpace(line)
if len(trimmed) == 0 {
if len(trimmed) == 0 || strings.HasPrefix(trimmed, "//") {
Copy link
Member

Choose a reason for hiding this comment

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

Mind adding a test into fileformat_test.go?

@github-actions github-actions bot added the area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) label Dec 10, 2021
@bryanhuhta
Copy link
Contributor Author

When writing the tests, a few thoughts came to me:

  • Is there any recommendation around using testdata/ to store test data?
  • I used memdb.NewMemdbDatastore to build a datastore.Datastore for my test. I'm not sure if there's an existing datastore.Datastore implementation specifically for being mocked in tests.

@josephschorr
Copy link
Member

When writing the tests, a few thoughts came to me:

  • Is there any recommendation around using testdata/ to store test data?

We actually do something similar for the consistency tests: https://github.com/authzed/spicedb/blob/main/internal/services/consistency_test.go (it loads from a directory called testconfigs), so follow the style in there. If the test data is small enough, you can also inline.

  • I used memdb.NewMemdbDatastore to build a datastore.Datastore for my test. I'm not sure if there's an existing datastore.Datastore implementation specifically for being mocked in tests.

MemDB is the correct datastore to use. See https://github.com/authzed/spicedb/blob/main/internal/services/consistency_test.go#L64 for an example

josephschorr
josephschorr previously approved these changes Dec 13, 2021
View reviewed changes
Copy link
Member

@josephschorr josephschorr left a comment

Choose a reason for hiding this comment

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

LGTM

Ignore comments when loading relationship tuples in test mode
b1257e1 
Signed-off-by: Bryan Huhta <bryanhuhta@github.com>
josephschorr
josephschorr approved these changes Dec 13, 2021
View reviewed changes
Copy link
Member

@josephschorr josephschorr left a comment

Choose a reason for hiding this comment

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

LGTM

@josephschorr josephschorr merged commit 0607f4e into authzed:main Dec 13, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Dec 13, 2021
@bryanhuhta bryanhuhta deleted the bryanhuhta/parse-comments branch December 13, 2021 20:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@josephschorr josephschorr josephschorr approved these changes

Assignees
No one assigned
Labels
area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

SpiceDB server cannot parse comments in the relationships section
2 participants
@bryanhuhta @josephschorr