Skip to content

MI: reinstate nvme_mi_init_ep for endpoint initialisation in test#400

Merged
igaw merged 1 commit intolinux-nvme:masterfrom
CodeConstruct:mi-test-ep-init
Jun 23, 2022
Merged

MI: reinstate nvme_mi_init_ep for endpoint initialisation in test#400
igaw merged 1 commit intolinux-nvme:masterfrom
CodeConstruct:mi-test-ep-init

Conversation

@jk-ozlabs
Copy link
Copy Markdown
Collaborator

Ideally, we would be using the actual implementation of
nvme_mi_init_ep() for our tests, rather than open-coding it in the test
init.

This change exports the nvme_mi_init_ep from libnvme-mi.so, but it
remains excluded from the headers, as it's only intended for use in the
transport API. This means we can call it from the tests, but keep is
somewhat-internal.

We put this into a specific _TEST section of the version script, to keep
separate from the public symbols.

This prevents us from diverging the endpoint init process in our
testcases.

Signed-off-by: Jeremy Kerr jk@codeconstruct.com.au

@codecov-commenter
Copy link
Copy Markdown

Codecov Report

Merging #400 (76a7472) into master (b7fe911) will increase coverage by 0.06%.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##           master    #400      +/-   ##
=========================================
+ Coverage    7.61%   7.68%   +0.06%     
=========================================
  Files          29      29              
  Lines        4414    4413       -1     
  Branches      828     828              
=========================================
+ Hits          336     339       +3     
+ Misses       3937    3933       -4     
  Partials      141     141              
Impacted Files Coverage Δ
test/mi.c 67.10% <100.00%> (-0.22%) ⬇️
src/nvme/mi.c 35.29% <0.00%> (+1.56%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b7fe911...76a7472. Read the comment docs.

@igaw
Copy link
Copy Markdown
Collaborator

igaw commented Jun 23, 2022

Do you want to add the crc32 method as well? That would make my coverage hack go away :)

@igaw
Copy link
Copy Markdown
Collaborator

igaw commented Jun 23, 2022

And please add a fat comment in the map file to warn anyone depending on this symbol!

@jk-ozlabs
Copy link
Copy Markdown
Collaborator Author

OK, will add crc32 too, good plan. Means we'll be testing more of the actual code.

Ideally, we would be using the actual implementation of
nvme_mi_init_ep() and nvme_mi_crc32_update for our tests, rather than
open-coding it in the test init.

This change exports nvme_mi_init_ep and nvme_mi_crc32_update from
libnvme-mi.so, but both remain excluded from the headers, as they are
only intended for use in the transport API. This means we can call them
from the tests, but keep somewhat-internal.

We put this into a specific _TEST section of the version script, to keep
separate from the public symbols, and add a comment to suit.

This prevents us from diverging the endpoint init process in our
testcases.

Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>
@igaw igaw merged commit 8556da5 into linux-nvme:master Jun 23, 2022
@igaw
Copy link
Copy Markdown
Collaborator

igaw commented Jun 23, 2022

Just got another idea: What about just have a separate library linked for testing only?

libnvme_mi_test = library(
   [...]
   link_args: ['-Wl,--version-script=' + mi_version_script_arg],
   [...]
)

and only have the internal symbols exposed in libnvme_mi_test (updating the mi_version_script_arg)

@jk-ozlabs
Copy link
Copy Markdown
Collaborator Author

Yep, could do - we could even drop --version-script for the libnvme-mi-test.so build, making all symbols global, perhaps.

This is all conditional on the coverage getting detected across the two builds though, I guess. Any reliable way to test this?

@jk-ozlabs jk-ozlabs deleted the mi-test-ep-init branch June 23, 2022 11:14
@igaw
Copy link
Copy Markdown
Collaborator

igaw commented Jun 23, 2022

I think you run

rm -rf obj && meson setup obj -Db_coverage=true --werror && meson test -C obj && ninja -C obj coverage --verbose

locally you should see the coverage report. I assume it will work as the source files match?

@jk-ozlabs
Copy link
Copy Markdown
Collaborator Author

Yep, that seems to work well. Proposed in #403

@igaw
Copy link
Copy Markdown
Collaborator

igaw commented Jun 23, 2022

Cool thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants