-
Notifications
You must be signed in to change notification settings - Fork 7
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
Extend unit test for existing functions #23
Conversation
For the commit and first message I would recommend something like: "Add unit tests for existing functions" or "Extend unit test coverage: (non)required repos/sw/pkgs" or something like that. |
f59de3b
to
4473781
Compare
LGTM, just those minor things that YaST developers will know better :) |
@mvidner @jreidinger if you have some time, kindly review. |
Pull Request Test Coverage Report for Build 938911081Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
spec/OneClickInstall_spec.rb
Outdated
filename = File.join(DATA_PATH, "serialized_result.xml") | ||
subject.ToXML(filename) | ||
expected = File.join(DATA_PATH, "serialized_expected.xml") | ||
expect(IO.read(filename)).to eql(IO.read(expected)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just note that can be quite fragile as identical XML is not necessary same as same content of xml due to formatting. Maybe comparing parsed xml trees is better way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to replace it with expect(REXML::Document.new(filename)).to eq(REXML::Document.new(expected))
but doesn't work. What did you have in mind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the end, I used file.read and it worked but still not using the xml tree. Another option would be to check the result partially like:
expect(file).to include("key").exactly(12).times
software.each_key{|key| expect(file).to include(key)}
repositories.each_key{|key| expect(file).to include(key)}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like it is not so easy. I found this https://stackoverflow.com/questions/30088334/compare-rexml-elements-for-name-attribute-equality-in-rspec so probably not worth it.
GetRequiredRepositories GetNonRequiredRepositories SetNonRequiredRepository SetRequiredRepositories GetRequiredSoftware GetRequiredPackages GetRequiredPatterns GetRequiredRemoveSoftware HaveAnyRecommended Load makeXMLFriendly fromXMLFriendly ToXML FromXML
InstallPackages InstallPatterns RemovePackages
✔️ Public Jenkins job #16 successfully finished |
✔️ Internal Jenkins job #7 successfully finished |
Adding functions to unit test
OneClickInstall:
GetRequiredRepositories
GetNonRequiredRepositories
SetNonRequiredRepository
SetRequiredRepositories
GetRequiredSoftware
GetRequiredPackages
GetRequiredPatterns
GetRequiredRemoveSoftware
HaveAnyRecommended
Load
makeXMLFriendly
fromXMLFriendly
ToXML
FromXML
OneClickInstallWorkerFunctions:
InstallPackages
InstallPatterns
RemovePackages