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

New test cases added #5

Closed
wants to merge 4 commits into from
Closed

New test cases added #5

wants to merge 4 commits into from

Conversation

harinandana02
Copy link
Contributor

Added several test cases ( opinionatedtest.jl file)

@filchristou filchristou self-requested a review May 3, 2023 12:03
@filchristou
Copy link
Member

Hi @harinandana02 . Thanks for the PR. Currently the test cases are failing. Could you investigate what's going wrong ? Also, it would be nice if you could provide a single commit (collapsing all your commits into 1) and a small description of what this PR does.

@codecov-commenter
Copy link

Codecov Report

Patch coverage has no change and project coverage change: +1.56 🎉

Comparison is base (0c548f1) 59.37% compared to head (6eb9850) 60.93%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@            Coverage Diff             @@
##             main       #5      +/-   ##
==========================================
+ Coverage   59.37%   60.93%   +1.56%     
==========================================
  Files           3        3              
  Lines         128      128              
==========================================
+ Hits           76       78       +2     
+ Misses         52       50       -2     
Flag Coverage Δ
unittests 60.93% <ø> (+1.56%) ⬆️

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

Impacted Files Coverage Δ
src/opinionated.jl 67.05% <ø> (+2.35%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@filchristou filchristou left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. At the moment commenting out the the functionality in the OAttributeGraph constructor kind-of kills the purpose of the PR. If you would like, you could try again with the additional functionality integrated. Feel free to test it first locally!

@@ -6,11 +6,13 @@ version = "0.2.0"
[deps]
DocStringExtensions = "ffbed154-4ef7-542d-bbb7-c09d3a79fcae"
Graphs = "86223c79-3864-5bf0-83f7-82e725a168b6"
Revise = "295af30f-e4ad-537b-8983-00126c2a3abe"
Copy link
Member

Choose a reason for hiding this comment

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

please do not introduce unneeded dependencies. Revise and Run are both very useful packages but not inseparable from this package. Users can have them if they wish already installed in the general environment or whatever.

DocStringExtensions = "0.9"
Graphs = "1"
julia = "1.8"
Copy link
Member

Choose a reason for hiding this comment

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

please avoid unneeded modifications like this

test/runtests.jl Outdated Show resolved Hide resolved

# unit testing
#simple test case
@test true
Copy link
Member

Choose a reason for hiding this comment

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

this @test true is totally unneeded. However I am having trouble to release what is the purpose of this testing file. It looks more or less like a repetition from the ./test/attributes.jl (?)

@harinandana02
Copy link
Contributor Author

Outdated pull request.

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.

3 participants