Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Fix pytest errors test collection #3972
Fix pytest errors test collection #3972
Changes from all commits
128acc8
1b462d4
6112ee1
c0ed0bd
f88ebb0
6ff260a
9e25b39
a6952aa
bc18cc2
eb4b777
b6c8cc9
0c2c3ba
5cc93ce
804b25b
2a98192
064e064
f024c10
79ef471
d725a51
0c77871
297b91b
1184898
3a08094
1682d90
79ce775
20decee
7bea695
8775e37
7dacbca
22ff92e
1f5ab15
bb079ab
9594d0f
2b7e163
9839e3a
fb9b65f
84cd718
b9a63c4
49fbed7
032e924
29933f6
dad023b
07dda70
1255915
3517a33
5c1447c
7ec2e0d
b9e2df8
a178467
b17254f
3e4f69e
6221722
d9d5f23
65ac11b
da2f76b
8c7a75e
d911eb3
9a52620
8fcfe5b
2b9e4ff
9637846
fa4af39
a674409
3be690f
fefd573
a93b953
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Specify an exception
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.
EDIT: Read the original conversation.
Why are you returning 'N/A' and not False or something more descriptive? Could we choose a better way here?
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.
Due to the function is called get_version it maybe has not sense to return false, but I agree this is could lead into errors in the future. However I can not see a perfect solution here, due to legacy code.
Maybe something like the following could make the job:
It is not the perfect solution, but we need to make this function not fail in case of failure gathering the version.
Please leave some feedback about the proposed solution in order to apply the changes
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 totally get what you mean! IMHO, I believe the function should definitely serve its purpose, and if it can't, it ought to throw an error. The object, function, or whatever made the call to it should handle the error gracefully (just in case it fails for some reason). That way, it can do whatever it needs to do. But remember, this utility has a clear goal, which, in my opinion, is to get the Wazuh version.
But, one more thing to consider, as you mentioned earlier, is the legacy code. So, I reckon your proposal will work as a temporary fix.
Please, apply it and I'll resolve this conversation.
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.
Why
plat
and notplatform
?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.
This is not related to the development. Changes only are aimed to ignore the platform deselection in case of using the defined variable
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.
Alright, I understand your point, but I'm actually referring to the name of the variable. You added it, so I was wondering if it's possible to change it to something more descriptive.
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.
Could you please explain to me why are you using this timeout?
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 did not choose the timeout; rather, I aimed to adhere to the current state of the tests as closely as possible during this development. Therefore, I utilized the already defined timeout of the test in this particular case. This timeout was defined hardcoded in the call for change_timeout.