python vfrcompiler tool #102
Conversation
This python VfrCompiler tool is the python implementation of the edk2 VfrCompiler tool which C implementation locates at https://github.com/tianocore/edk2/tree/master/BaseTools/Source/C/VfrCompile. This python implementation not only covers the same usage as the C version VfrCompiler, but also extends several new features. Edk2 Basetools issue link: tianocore#68 Cc: Rebecca Cran <[email protected]> Cc: Liming Gao <[email protected]> Cc: Bob Feng <[email protected]> Signed-off-by: Yuting Yang <[email protected]> Signed-off-by: Yuwei Chen <[email protected]>
This patch add the python vfrcompiler unit test cases. Cc: Rebecca Cran <[email protected]> Cc: Liming Gao <[email protected]> Cc: Bob Feng <[email protected]> Signed-off-by: Mingyue Liang <[email protected]> Signed-off-by: Yuwei Chen <[email protected]>
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #102 +/- ##
==========================================
- Coverage 4.27% 0.01% -4.27%
==========================================
Files 182 171 -11
Lines 81605 102689 +21084
==========================================
- Hits 3490 13 -3477
- Misses 78115 102676 +24561
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
BaseTools: Add python vfrcompiler unit test cases This patch add the python vfrcompiler unit test cases. Cc: Rebecca Cran <[email protected]> Cc: Liming Gao <[email protected]> Cc: Bob Feng <[email protected]> Signed-off-by: Mingyue Liang <[email protected]> Signed-off-by: Yuwei Chen <[email protected]>
There was a problem hiding this comment.
The information you have here is insufficient for me to understand what testing you have done, how to repeat your testing, and how i would add my own testing and validation.
Additionally this is written against the source code in the edk2 format and thus even when merged into this project it will not work?
There was a problem hiding this comment.
Hi, Sean Brogan, we update the README.md as follows
Python Vfrcompiler Unit test
OverView
The feature is the implementation of unit test for the python vfrcompiler tool.
Introduction
We choose ten VFR files from the following modules in the edk2 as test cases.
- IScsiDxe
- VlanConfigDxe
- Platform
- BootMaintenanceManagerUiLib
- BootManagerUiLib
- DeviceManagerUiLib
- FileExplorerLib
- Ip4Dxe
- UiApp
- RamDiskDxeBoot
The main test function is in edk2\BaseTools\Source\Python\tests\vfr_yaml_compiler\test_Vfrcompiler.py
We do the testing from the following two aspects.
- Function test
- We add function test for the PreProcess() / Compile() / GenBinaryFiles() / DumpSourceYaml() functions of the tool.
- Output test
- We add the test_vfr_lst_file () function to do the comparsions.
- To differentiate from the original C tool output files, the python tool generated files are named with prefix 'PyVfr' in DEBUG directory.
- We compare the python tool generated IFR file with the C tool generated ones to valid the effectiveness of the new tool.
Add test cases
If you want to add more test cases to do further validation, please do the following steps.
-
Open edk2\BaseTools\Source\Python\tests\pytest_vfrcompiler.ini
-
Add the module you want to test in
target_test_folderstarget_test_folders = Module Name you want to test
Run test
To apply the unit test feature, please do the following steps.
- Refer to edk2\BaseTools\Source\Python\VfrCompiler\README.md and run build command.
- Add Env: run
pip install pytestbased on the original build environment. - run
cd edk2\BaseTools\Source\Python\tests - open edk2\BaseTools\Source\Python\tests\pytest.ini and Modify the parameters that need to be used
python_files =
test_Vfrcompiler.py
This parameter selects the test file.
- run command
pytestin edk2\BaseTools\Source\Python\tests
There was a problem hiding this comment.
I think that is helpful but I don't think your testing strategy as listed is maintainable.
I would hope that the expectation is that this is tested regularly. This looks very manual and fragile given it is running the vfr compile on edk2. I would suggest we discuss doing something like pytools integration tests. https://github.com/tianocore/edk2-pytool-extensions/tree/master/tests.integration
I also think you need unit tests that are faster and don't depend on external resources. There should be a file with one simple test for each optcode/statement with known ins/outs.
Finally, this should be run like a standard python project rather than a folder in edk2 repo that has been transplanted.
There was a problem hiding this comment.
I am on vacation for a while. Maybe @Javagedes can provide some feedback in the short term and/or discuss at the weekly tools sync.
There was a problem hiding this comment.
Overall, I only found a few bugs that could actually be a problem. This was mainly accidently using a comparison instead of a variable set (i.e. var == value instead of var = value)
I did still find multiple issues I'm concerned about. The ones that stand out are:
- You should not use bare
except:statements. Always, at a minimum, useexcept Exception:. Using bare except statements cause many issues, but one of the more notable ones is stalls in the code when someone tries to exit with Ctrl+C as a bare except statement catches that, then generally completely breaks. - You do broad imports with "*" at the top of files. You should really only import exactly what you need for that file.
- You use the old style of inserting variables into strings with
.format(). The standard way is to use f-strings (i.e.f"insert a variable here: {variable}" - Comaprisons to
None,TrueandFalseshould useisandis notinstead of==and!=
I apologize in advance, many of the comments tend to be the same thing over and over again. In the smaller files I tried to catch all of them. In the larger ones I left comments at the top of the file to reduce the amount of spam comments in the file.
| Data4[6] = int(AsciiGuidBuffer[32:34], 16) | ||
| Data4[7] = int(AsciiGuidBuffer[34:36], 16) | ||
| except: | ||
| EdkLogger.error("VfrCompiler", PARAMETER_INVALID, "Invalid Data value!") |
There was a problem hiding this comment.
Edk2Logger should be directly imported in this file.
There was a problem hiding this comment.
Hi Javagedes, thank you very much for your valuable review comments. 😊
We will commit to addressing the issues and propose a new pull request after making all modifications~
|
|
||
| # Verify the correct number of items were scanned. | ||
| if Index != 11: | ||
| EdkLogger.error("VfrCompiler", PARAMETER_INVALID, "Invalid option value") |
There was a problem hiding this comment.
Edk2Logger should be directly imported in this file.
| continue | ||
|
|
||
| if Index < 36: | ||
| EdkLogger.error("VfrCompiler", PARAMETER_INVALID, "Invalid option value") |
There was a problem hiding this comment.
Edk2Logger should be directly imported in this file.
| else: | ||
| if ( |
There was a problem hiding this comment.
This should probably be refactored to be an elif and then else statement
| Data4[5] = int(AsciiGuidBuffer[30:32], 16) | ||
| Data4[6] = int(AsciiGuidBuffer[32:34], 16) | ||
| Data4[7] = int(AsciiGuidBuffer[34:36], 16) | ||
| except: |
There was a problem hiding this comment.
You should never use a bare except statement. This can catch extra exceptions like KeyboardInterrupt and SystemExit which can lock up your code. You should always do except Exception at the bare minimum
| ] | ||
|
|
||
|
|
||
| class VfrVarDataTypeDB(object): |
There was a problem hiding this comment.
| class VfrVarDataTypeDB(object): | |
| class VfrVarDataTypeDB: |
| self.Next = None | ||
|
|
||
|
|
||
| class SVfrDataField(object): |
There was a problem hiding this comment.
| class SVfrDataField(object): | |
| class SVfrDataField: |
| return False | ||
|
|
||
|
|
||
| class SVfrDataType(object): |
There was a problem hiding this comment.
| class SVfrDataType(object): | |
| class SVfrDataType: |
| return True | ||
| elif self.Identifier == None: | ||
| return False | ||
| elif self.Identifier == Identifier: |
There was a problem hiding this comment.
Just return the identifier comparison
| elif self.Identifier == Identifier: | |
| return self.Identifier == Identifier |
| @@ -0,0 +1,2238 @@ | |||
| import ctypes | |||
There was a problem hiding this comment.
Should use is or is not when comparing variables with None, True, False throughout this file.
|
The PR which is updated following the feedback is: |
BaseTools: Python VfrCompiler implementation
This python VfrCompiler tool is the python implementation
of the edk2 VfrCompiler tool which C implementation locates at
https://github.com/tianocore/edk2/tree/master/BaseTools/Source/C/VfrCompile.
This python implementation not only covers the same usage as
the C version VfrCompiler, but also extends several new features.
Edk2 Basetools issue link:
#68
Cc: Rebecca Cran [email protected]
Cc: Liming Gao [email protected]
Cc: Bob Feng [email protected]
Signed-off-by: Yuting Yang [email protected]
Signed-off-by: Yuwei Chen [email protected]