PYSCAN-4: Add tests for environment #5
Conversation
joke1196
force-pushed
the
DAVID/PYSCAN-4
branch
from
864dad9
to
2327215
Compare
joke1196
requested review from
jdodinh and
guillaume-dequenne-sonarsource
and removed request for
jdodinh and
guillaume-dequenne-sonarsource
| log.setLevel(logging.INFO) | ||
| handler = logging.StreamHandler() | ||
| handler.terminator = "" | ||
| log.addHandler(handler) |
There was a problem hiding this comment.
I have added a ticket to use this logger in other parts of the application. i.e the scanner
|
I didn't think it was strictly necessary to test all the other functions |
There was a problem hiding this comment.
Looks good overall, left some questions.
Minor feedback: the ordering of commits did not help with the review here: first commit uses the ApplicationLogger which is introduced in the second commit. also, it would have been nice to have the formatting changes in a separate commits from the actual changes.
src/py_sonar_scanner/environment.py
Outdated
| @@ -17,67 +17,90 @@ | |||
| # along with this program; if not, write to the Free Software Foundation, | |||
| # Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. | |||
| # | |||
| from logging import Logger | |||
There was a problem hiding this comment.
This import is unused following the introduction of ApplicationLogger.
src/py_sonar_scanner/environment.py
Outdated
| with open(destination, "wb") as output: | ||
| output.write(scanner_res.read()) | ||
|
|
||
| def _unzip_binaries(self, scanner_zip_path: str, destination: str): |
There was a problem hiding this comment.
Should we consider making turning these simple methods into functions, as they don't really use anything from the class? (I guess that would make us revisit the way the logger works as well though - not sure about that).
There was a problem hiding this comment.
I moved the last two methods to a utils file. But not the _download_scanner_binraies as it is still too strongly linked to the environment. I created a ticket so that we do not forget.
| from logging import Logger | ||
| from typing import Optional | ||
|
|
||
| class ApplicationLogger(): |
There was a problem hiding this comment.
Just so I understand, the point of having this class is to wrap a single logging logger associated with main in the whole application?
There was a problem hiding this comment.
Yes, exactly this was so that the main configuration would be the same everywhere.
| def _setup_logger(log: Logger): | ||
| log.setLevel(logging.INFO) | ||
| handler = logging.StreamHandler() | ||
| handler.terminator = "" |
There was a problem hiding this comment.
Why do we need to set terminators to ""? Is is to ensure this can run into multiple environments? Aren't the newlines needed to prevent logs from being merged together in a single line?
There was a problem hiding this comment.
When processing the logs from SonarScanner we would need to set this terminator in order to not have blank lines for each line of logs. we had to do it already when using print. #4 . Now I am thinking if there could be a way the logs would get merged together if they are not flushed directly. Not sure yet.
There was a problem hiding this comment.
Thanks for clarifying. So indeed when dealing with logs from the wrapped scanner, they already contain the newline token and we should make sure to not add it again? But when we produce our own, we should still have one?
Anyway, I guess we can add the proper testing for the various scenarios when implementing PYSCAN-28.
| @@ -20,7 +20,8 @@ classifiers = [ | |||
| ] | |||
|
|
|||
| dependencies = [ | |||
| "toml>=0.10.2", | |||
| "toml>=0.10.2", | |||
| "pyfiglet" | |||
|
Sorry about this mix-up in the commit history I am not sure what I did here but it is certainly wrong. |
You are welcome, just don't get inspired by my commit history :). |
joke1196
force-pushed
the
DAVID/PYSCAN-4
branch
from
56c0861
to
aacfb52
Compare
No description provided.