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

build: Add tests for fetch using passing arguments #115

Merged
merged 12 commits into from
Jan 15, 2024

Conversation

sbshah97
Copy link
Contributor

Added 5 different test cases to test the fetch functionality.
The remaining tests need to be changed once the functionality is fixed at the root source

Closes #40

Copy link
Contributor

@phughk phughk left a comment

Choose a reason for hiding this comment

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

Great work, thank you for doing this! Left some comments for things to improve.

db_test.go Outdated Show resolved Hide resolved
db_test.go Outdated Show resolved Hide resolved
db_test.go Outdated Show resolved Hide resolved
db_test.go Outdated Show resolved Hide resolved
db_test.go Outdated Show resolved Hide resolved
db_test.go Outdated Show resolved Hide resolved
- For negative test cases, test cases have been skipped for now.
- For passing test cases, a helper method has been added to validate output
@sbshah97 sbshah97 mentioned this pull request Dec 1, 2023
2 tasks
@sbshah97 sbshah97 requested a review from phughk December 1, 2023 15:19
Copy link
Contributor

@phughk phughk left a comment

Choose a reason for hiding this comment

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

Minor requests thank you!

db_test.go Outdated Show resolved Hide resolved
db_test.go Outdated Show resolved Hide resolved
@ElecTwix
Copy link
Contributor

ElecTwix commented Dec 7, 2023

Maybe we can use unmarshalRaw or smartUnmarshal to make it simpler because there is a lot of casting.

@sbshah97
Copy link
Contributor Author

sbshah97 commented Dec 7, 2023

Maybe we can use unmarshalRaw or smartUnmarshal to make it simpler because there is a lot of casting.

Yep I was thinking of doing it in the follow up PR once this goes in.
Can you help me with how those would be used here?

I am not sure how to use those methods, at the end of the README there were some examples but didn't really help.

@phughk
Copy link
Contributor

phughk commented Dec 7, 2023

We should wait for the unmarshal improvements before merging this. Appreciate the patience and diligence 👍

add smartUnmarshal as helper in fetch tests
@ElecTwix
Copy link
Contributor

ElecTwix commented Dec 7, 2023

opened a PR to @sbshah97 's repo sbshah97#1 for unmarshal.

@phughk phughk self-assigned this Dec 8, 2023
db_test.go Outdated Show resolved Hide resolved
db_test.go Outdated Show resolved Hide resolved
db_test.go Outdated Show resolved Hide resolved
db_test.go Outdated Show resolved Hide resolved
db_test.go Outdated Show resolved Hide resolved
db_test.go Outdated Show resolved Hide resolved
db_test.go Outdated Show resolved Hide resolved
db_test.go Outdated Show resolved Hide resolved
db_test.go Outdated Show resolved Hide resolved
db_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@phughk phughk left a comment

Choose a reason for hiding this comment

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

Some minor comments but its nearly ready :)

sbshah97 and others added 4 commits January 5, 2024 16:51
Code Review comments suggestions added

Co-authored-by: Przemyslaw Hugh Kaznowski <hughkaznowski@protonmail.com>
@sbshah97
Copy link
Contributor Author

sbshah97 commented Jan 7, 2024

@phughk could you review this again?

@timpratim
Copy link
Contributor

Looks good. Thanks @sbshah97 !. Merging it.

@timpratim timpratim merged commit e78588e into surrealdb:main Jan 15, 2024
2 checks passed
@sbshah97 sbshah97 deleted the dev/sbshah97/db_test branch January 15, 2024 22:52
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.

Getting Parsing Error When Use Fetch with map[string]interface
4 participants