Skip to content
This repository was archived by the owner on Mar 16, 2026. It is now read-only.

Basic CI/CD#16

Merged
Wenzel merged 7 commits intoIntelLabs:masterfrom
Wenzel:basic_ci_cd
Oct 11, 2022
Merged

Basic CI/CD#16
Wenzel merged 7 commits intoIntelLabs:masterfrom
Wenzel:basic_ci_cd

Conversation

@Wenzel
Copy link
Copy Markdown
Contributor

@Wenzel Wenzel commented Oct 3, 2022

New makefile targets

  • lint: runs flake8
  • lint_check: runs flake8 and count the number of errors

Tools used:

  • flake8

Configuration

  • max-line-length: 120
  • select: F401 and F841 (unused imports and variable assignments)

There are so many lint warnings and error that it's a better strategy to select and enable lint categories one by one in multiple PRs.

@Wenzel Wenzel force-pushed the basic_ci_cd branch 6 times, most recently from 7354731 to e937e47 Compare October 4, 2022 13:23
@Wenzel Wenzel requested a review from il-steffen October 4, 2022 13:26
Copy link
Copy Markdown
Contributor

@il-steffen il-steffen left a comment

Choose a reason for hiding this comment

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

There's a few places where the auto-formating hurts readability...some places where we really should not do this without prior refactoring, I think...

node.node_struct["info"]["trimmed"] = True
node.set_score(self.scheduler.score_speed(node))
if results.get("performance"):
oldperf = node.get_initial_performance()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

breaks the following out-commented print...either remove all or comment all?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

the print() could be converted to logger.debug()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

commented-out code is dead code,
I can convert it to logger.debug() for convenience.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

In my last update, I uncommented the print() dead code, and turned it into a logger.debug() statement, keeping the 2 lines above of course

@il-steffen
Copy link
Copy Markdown
Contributor

one small tidbid....the changes look fine otherwise but we should triple-check that the redqueen changes are really correct (eg. no side-effect to the called functions..)

@Wenzel
Copy link
Copy Markdown
Contributor Author

Wenzel commented Oct 9, 2022

one small tidbid....the changes look fine otherwise but we should triple-check that the redqueen changes are really correct (eg. no side-effect to the called functions..)

The only way to ensure that redqueen works is to have unit tests, otherwise we can't allow ourselves a simple refactoring because we are afraid of silent breaking changes.

I've seen the redqueen_mut.py test file.
What's the status of this ?
Can we include these tests in the CI as well ?
How much coverage does it have on the redqueen codebase ?

@il-steffen
Copy link
Copy Markdown
Contributor

il-steffen commented Oct 10, 2022

I've seen the redqueen_mut.py test file.
What's the status of this ?
Can we include these tests in the CI as well ?
How much coverage does it have on the redqueen codebase ?

I never ran this code, just kept it around in case we ever get to this kind of testing. I think we can use it with input from a random fuzzing session to at least produce some known-good test vectors. This would be far from actual functional testing, but better than looking for bugs through manual execution..

The inputs for this function are probably the files generated at $workdir/redqueen* during an active fuzzing run.

@Wenzel Wenzel merged commit 5ec7243 into IntelLabs:master Oct 11, 2022
@Wenzel Wenzel deleted the basic_ci_cd branch October 11, 2022 13:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants