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

Sqlite test #7

Open
wants to merge 23 commits into
base: dev
Choose a base branch
from
Open

Sqlite test #7

wants to merge 23 commits into from

Conversation

Farreeda
Copy link
Collaborator

@Farreeda Farreeda commented Mar 17, 2023

Fixing the file_path issue and testing the _dbconnect function for SQLite only
issue #3 & issue #6

Copy link
Collaborator

@TheCedarPrince TheCedarPrince left a comment

Choose a reason for hiding this comment

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

Hi @Farreeda ,

Thank you so much for working on this issue! I left some comments for improving the test suite here and some feedback.

Let me know if you have any questions!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you remove this file from this PR? It should not be here but was autogenerated by VSCode. Feel free to update the .gitignore file to ignore all things generated from VSCode.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you remove this file from this PR? It should not be here but was autogenerated by VSCode. Feel free to update the .gitignore file to ignore all things generated from VSCode.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The reason why this PR's test are failing right now is due to the fact that _dbconnect is trying to access the DB file but the DB file is not being sourced properly from the test environment. You will have to add in the db file to the test environment, like test/data/sqlite_test.db and then have the test environment find the file.

This is also why you got the error email is that I enabled the test suite to run and GitHub sent you an email to say that the test suite failed.

Let me know if you have any questions -- good start! :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is updated, thank you for your patience :D

removed .vscode folder
added
Copy link
Collaborator

@TheCedarPrince TheCedarPrince left a comment

Choose a reason for hiding this comment

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

Good work! Only a few small tweaks and this can be merged!

test/runtests.jl Outdated
Comment on lines 3 to 4
@test hello("Julia") == "Hello, Julia"
@test domath(2.0) ≈ 7.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you remove this?

test/runtests.jl Outdated

@testset "_dbconnect function for SQLite" begin

conn= _dbconnect(SQLite.DB, "DBConnector.jl/test/data/database.db")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Small think but could you rename "database.db" to "sqlite.db"?

@testset "_dbconnect function for SQLite" begin

conn= _dbconnect(SQLite.DB, "DBConnector.jl/test/data/database.db")
@test @isdefined conn
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is a good test but could you also write a test that:

  1. Queries the database
  2. Reports a result
  3. Verifies that the result returns what is expected

@TheCedarPrince TheCedarPrince changed the base branch from main to dev June 25, 2023 18:53
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