Attention is currently required from: Thomas Heijligen, Anastasia Klimchuk.
Evan Benn has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/69268 )
Change subject: tests: Add llvm-cov option and run target for code coverage
......................................................................
Patch Set 9:
(2 comments)
File meson.build:
https://review.coreboot.org/c/flashrom/+/69268/comment/bff36be2_eec15379
PS5, Line 534: if get_option('llvm_cov')
> Also need to check that tests feature is enabled
coverage can be used independently of unit tests
File meson_options.txt:
https://review.coreboot.org/c/flashrom/+/69268/comment/a0029578_f31bbc9d
PS5, Line 19: type : 'boolean'
> I think this should be a feature, like unit tests themselves. […]
Done
--
To view, visit https://review.coreboot.org/c/flashrom/+/69268
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Id6c73bff46e7b88d425956a80def97082b201f56
Gerrit-Change-Number: 69268
Gerrit-PatchSet: 9
Gerrit-Owner: Evan Benn <evanbenn(a)google.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Mon, 14 Nov 2022 00:31:12 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Thomas Heijligen, Anastasia Klimchuk.
Evan Benn has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/69266 )
Change subject: tests: Detect gcov file io and unmock io functions
......................................................................
Patch Set 9:
(4 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/69266/comment/4e6d1646_5a55fd70
PS6, Line 9: atexit
> missing space: atexit -> at exit
atexit is the name of the functionality through which code coverage writes data to disk. I will reword it.
File tests/io_mock.h:
https://review.coreboot.org/c/flashrom/+/69266/comment/82153d1d_f07d844b
PS6, Line 110: size_t (*iom_fwrite)(void *state, const void *buf, size_t size, size_t len, FILE *fp);
> Adding these (iom_fwrite and iom_fdopen) two can go into a separate patch, together with their calls […]
Done
File tests/tests.c:
https://review.coreboot.org/c/flashrom/+/69266/comment/f89ee844_f8e7ea91
PS6, Line 52: real_io_mock
> I think you can drop _mock, this is just real_io
Done
https://review.coreboot.org/c/flashrom/+/69266/comment/f34f982a_73674cc4
PS6, Line 29: static int unwrap_open(void *state, const char *pathname, int flags, mode_t mode)
: {
: LOG_ME;
: (void)state;
: return __real_open(pathname, flags, mode);
: }
:
: static FILE *unwrap_fdopen(void *state, int fd, const char *mode)
: {
: LOG_ME;
: (void)state;
: return __real_fdopen(fd, mode);
: }
:
: static size_t unwrap_fwrite(void *state, const void *ptr, size_t size, size_t nmemb, FILE *fp)
: {
: LOG_ME;
: (void)state;
: return __real_fwrite(ptr, size, nmemb, fp);
: }
:
: // Mock ios that defer to the real io functions.
: // These exist so that code coverage can print to real files on disk.
: static const struct io_mock real_io_mock = {
: .iom_open = unwrap_open,
: .iom_fwrite = unwrap_fwrite,
: .iom_fdopen = unwrap_fdopen,
: };
> Ideally, if this could go to a separate file for example `coverage. […]
named it unmock_io as its what it does, I think it will be useful for more than just coverage.
--
To view, visit https://review.coreboot.org/c/flashrom/+/69266
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: If06053ecd78e886c8f7fc55813f4b5635be78c6b
Gerrit-Change-Number: 69266
Gerrit-PatchSet: 9
Gerrit-Owner: Evan Benn <evanbenn(a)google.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Mon, 14 Nov 2022 00:30:29 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Evan Benn.
Hello build bot (Jenkins), Anastasia Klimchuk,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/69267
to look at the new patch set (#9).
Change subject: tests: Detect llvm coverage file io and unmock io functions
......................................................................
tests: Detect llvm coverage file io and unmock io functions
Code coverage writes data to disk, we need to unmock the file io at this
point so that the data is really written.
BUG=b:187647884
BRANCH=None
TEST=llvm-profdata merge -sparse default.profraw -o default.profdata
TEST=llvm-cov show ./flashrom_unit_tests
-instr-profile=default.profdata --format=html --output-dir=.
Change-Id: I21cc1d631e92fa19006b967e85676f108e80b307
Signed-off-by: Evan Benn <evanbenn(a)chromium.org>
---
M Documentation/building.md
M tests/tests.c
M tests/unmock_io.c
M tests/wraps.h
4 files changed, 52 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/67/69267/9
--
To view, visit https://review.coreboot.org/c/flashrom/+/69267
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I21cc1d631e92fa19006b967e85676f108e80b307
Gerrit-Change-Number: 69267
Gerrit-PatchSet: 9
Gerrit-Owner: Evan Benn <evanbenn(a)google.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Evan Benn <evanbenn(a)google.com>
Gerrit-MessageType: newpatchset
Attention is currently required from: Thomas Heijligen.
Hello build bot (Jenkins), Thomas Heijligen, Anastasia Klimchuk,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/69266
to look at the new patch set (#9).
Change subject: tests: Detect gcov file io and unmock io functions
......................................................................
tests: Detect gcov file io and unmock io functions
Code coverage writes data to disk, we need to unmock the file io at this
point so that the data is really written.
BUG=b:187647884
BRANCH=None
TEST=meson test
TEST=ninja coverage
Change-Id: If06053ecd78e886c8f7fc55813f4b5635be78c6b
Signed-off-by: Evan Benn <evanbenn(a)chromium.org>
---
M Documentation/building.md
M tests/meson.build
M tests/tests.c
A tests/unmock_io.c
A tests/unmock_io.h
M tests/wraps.h
6 files changed, 120 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/66/69266/9
--
To view, visit https://review.coreboot.org/c/flashrom/+/69266
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: If06053ecd78e886c8f7fc55813f4b5635be78c6b
Gerrit-Change-Number: 69266
Gerrit-PatchSet: 9
Gerrit-Owner: Evan Benn <evanbenn(a)google.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-MessageType: newpatchset
Attention is currently required from: Anastasia Klimchuk.
Evan Benn has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/69263 )
Change subject: tests: Mock the mode_t variant of open
......................................................................
Patch Set 6:
(4 comments)
File tests/io_mock.h:
https://review.coreboot.org/c/flashrom/+/69263/comment/463e3631_3bf22d6c
PS4, Line 37: #include <fcntl.h>
> Is this a platform-specific header? Is this header present on non-Linux environments? The unit tests […]
this one is posix, and posix says: `The <fcntl.h> header shall define the mode_t`
Not sure if we need to support non-posix, or what we can do if so.
File tests/tests.c:
https://review.coreboot.org/c/flashrom/+/69263/comment/2820ca3f_9eef68d8
PS4, Line 21: #include <fcntl.h>
> If you need to include the same header twice, this means it needs to be included on a higher level, […]
Done
https://review.coreboot.org/c/flashrom/+/69263/comment/97060a59_91af6f82
PS4, Line 103: mode_t mode = 0;
: if (flags & O_CREAT) {
: va_list ap;
: va_start(ap, flags);
: mode = va_arg(ap, mode_t);
: va_end(ap);
: }
> This code repeats for all variants of open wraps, can this code go to mock_open?
It has to be in the varargs function. On the other hand, I don't know the reason for the open64 and open64_2 functions, so maybe they can be removed?
File tests/wraps.h:
https://review.coreboot.org/c/flashrom/+/69263/comment/3bbdd5d0_042af44e
PS4, Line 31: int __wrap_open(const char *pathname, int flags, ...);
: int __wrap_open64(const char *pathname, int flags, ...);
: int __wrap___open64_2(const char *pathname, int flags, ...);
> Just to check, will the wraps still capture two-args variant of `open` calls?
yes, at link time there is no information about function type, only the name. This just tells the current translation unit that this is a vararg function.
--
To view, visit https://review.coreboot.org/c/flashrom/+/69263
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I8c134e6d36a248d0f51985e389085a9e585fb83d
Gerrit-Change-Number: 69263
Gerrit-PatchSet: 6
Gerrit-Owner: Evan Benn <evanbenn(a)google.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Mon, 14 Nov 2022 00:23:28 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Thomas Heijligen, Evan Benn.
Hello build bot (Jenkins), Thomas Heijligen, Anastasia Klimchuk,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/69268
to look at the new patch set (#8).
Change subject: tests: Add llvm-cov option and run target for code coverage
......................................................................
tests: Add llvm-cov option and run target for code coverage
Code coverage can be requested with -Dllvm_cov and run with ninja
llvm-cov.
BUG=b:187647884
BRANCH=None
TEST=meson test; ninja llvm-cov
Change-Id: Id6c73bff46e7b88d425956a80def97082b201f56
Signed-off-by: Evan Benn <evanbenn(a)chromium.org>
---
M Documentation/building.md
M meson.build
M meson_options.txt
A scripts/llvm-cov
M tests/meson.build
5 files changed, 42 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/68/69268/8
--
To view, visit https://review.coreboot.org/c/flashrom/+/69268
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Id6c73bff46e7b88d425956a80def97082b201f56
Gerrit-Change-Number: 69268
Gerrit-PatchSet: 8
Gerrit-Owner: Evan Benn <evanbenn(a)google.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Evan Benn <evanbenn(a)google.com>
Gerrit-MessageType: newpatchset
Attention is currently required from: Anastasia Klimchuk.
Hello Anastasia Klimchuk,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/69537
to look at the new patch set (#2).
Change subject: tests: Move test.h include
......................................................................
tests: Move test.h include
BUG=None
BRANCH=None
TEST=None
Change-Id: I8e0611c415c921f5b04b20270fb26e147fefd1b8
Signed-off-by: Evan Benn <evanbenn(a)chromium.org>
---
M tests/io_mock.c
M tests/io_mock.h
2 files changed, 16 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/37/69537/2
--
To view, visit https://review.coreboot.org/c/flashrom/+/69537
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I8e0611c415c921f5b04b20270fb26e147fefd1b8
Gerrit-Change-Number: 69537
Gerrit-PatchSet: 2
Gerrit-Owner: Evan Benn <evanbenn(a)google.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: newpatchset
Attention is currently required from: Thomas Heijligen, Evan Benn.
Hello build bot (Jenkins), Thomas Heijligen, Anastasia Klimchuk,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/69268
to look at the new patch set (#7).
Change subject: tests: Add llvm-cov option and run target for code coverage
......................................................................
tests: Add llvm-cov option and run target for code coverage
Code coverage can be requested with -Dllvm_cov and run with ninja
llvm-cov.
BUG=b:187647884
BRANCH=None
TEST=meson test; ninja llvm-cov
Change-Id: Id6c73bff46e7b88d425956a80def97082b201f56
Signed-off-by: Evan Benn <evanbenn(a)chromium.org>
---
M Documentation/building.md
M meson.build
M meson_options.txt
A scripts/llvm-cov
M tests/meson.build
5 files changed, 40 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/68/69268/7
--
To view, visit https://review.coreboot.org/c/flashrom/+/69268
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Id6c73bff46e7b88d425956a80def97082b201f56
Gerrit-Change-Number: 69268
Gerrit-PatchSet: 7
Gerrit-Owner: Evan Benn <evanbenn(a)google.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Evan Benn <evanbenn(a)google.com>
Gerrit-MessageType: newpatchset
Attention is currently required from: Evan Benn.
Hello build bot (Jenkins), Anastasia Klimchuk,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/69267
to look at the new patch set (#8).
Change subject: tests: Detect llvm coverage file io and unmock io functions
......................................................................
tests: Detect llvm coverage file io and unmock io functions
Code coverage writes data to disk, we need to unmock the file io at this
point so that the data is really written.
BUG=b:187647884
BRANCH=None
TEST=llvm-profdata merge -sparse default.profraw -o default.profdata
TEST=llvm-cov show ./flashrom_unit_tests
-instr-profile=default.profdata --format=html --output-dir=.
Change-Id: I21cc1d631e92fa19006b967e85676f108e80b307
Signed-off-by: Evan Benn <evanbenn(a)chromium.org>
---
M Documentation/building.md
M tests/tests.c
M tests/unmock_io.c
M tests/wraps.h
4 files changed, 54 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/67/69267/8
--
To view, visit https://review.coreboot.org/c/flashrom/+/69267
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I21cc1d631e92fa19006b967e85676f108e80b307
Gerrit-Change-Number: 69267
Gerrit-PatchSet: 8
Gerrit-Owner: Evan Benn <evanbenn(a)google.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Evan Benn <evanbenn(a)google.com>
Gerrit-MessageType: newpatchset