[Fuego] Procedure to commit the code

Tim.Bird at sony.com Tim.Bird at sony.com
Mon Nov 4 09:45:32 UTC 2019



> -----Original Message-----
> From: Kumar Thangavel 
> 
> Thanks a lot for your information Tim.
> 
> 
>   As we discussed earlier, I have added one Functional test cases
> "Functional.check_mounts". This is the initial version of the code. This will
> compare the expected mount points of boards with actual board mounted
> points. If any expected mount points is not mounted in the board, test will
> fail.
> 
> 
> Please find the attached patch file. Could you please review this and provide
> your valuable comments on this.

> Subject: [PATCH 1301/1301] This PATCH added test cases to check the expected
> mount points with system mount points, if any mount point is not matched test
> will fail. This is the Initial version of code with default spec.
 > Signed-off-by: Kumar Thangavel <thangavel.k at hcl.com>

Something weird happened here.  The entire commit message was put
into the subject line of the commit message.  Also, the patch number is
very high (1301)

This subject should have had the format:
[PATCH 1/1] 

I'm not sure how you generated the patch, but for the top-most commit
in a repository you normally do something like:
git format-patch HEAD

When making the commit, use 'git commit [-m <subject line>]'
You can use '-m <subject line>' to add the subject, and add the commit
message using the editor,
or place the subject line, an empty line, and the commit message in
the editor.

The subject line should start with the area of the project that this patch
affects (ending in a colon: '<area>:'  This can be 'core', 'docker', 'parser', etc., or in the case of a test
the test name (without the Functional/Benchmark prefix) should be used.
In your case the area prefix for the Subject line would be 'check_mounts:'

The Signed-off-by: line should be on a line by itself.

> ---
> tests/Functional.check_mounts/fs-test-1.1.tgz | Bin 0 -> 2209 bytes
> tests/Functional.check_mounts/fuego_test.sh   |  19 ++++++++++++++++++
> tests/Functional.check_mounts/spec.json       |   7 +++++++
> tests/Functional.check_mounts/test.yaml       |  15 ++++++++++++++
> 4 files changed, 41 insertions(+)
> create mode 100644 tests/Functional.check_mounts/fs-test-1.1.tgz
> create mode 100755 tests/Functional.check_mounts/fuego_test.sh
> create mode 100644 tests/Functional.check_mounts/spec.json
> create mode 100644 tests/Functional.check_mounts/test.yaml
>
> diff --git a/tests/Functional.check_mounts/fs-test-1.1.tgz b/tests/Functional.check_mounts/fs-test-1.1.tgz
> new file mode 100644
> index 0000000000000000000000000000000000000000..1f832d3c62f4b21b36c1148313c20841fae9ecdb
> GIT binary patch
> literal 2209
> zcmV;S2wwLeiwFQZDX?7t1MOLRbJ|D}_rK^<bP?YgdwKemDmzX^1`(^4hf9cLbB>D=
> zVg%X<jY^N$j?dqHyGIfd7$?~~@1|16RASJ~^snFDqrnO)Aq&DvquQu#{Y|5e<K at v2
> zeKuYm)$^a}*lHXc9Ua#94-XD;zR{>3?{C4;-z89fL_uiy09(v+zPj&Q>p$tRa{jl1
> z5q+<k&&=cN{%`Czj*nOTzyGp+yan}VmSgSsKlgua at B4-Xd$5QPsDR1mp5fc>6v7#U
> z;4TQ+94y;OzXxz*hck!*<^#L+m>IIk(mDbXW|Mn&zCE3VuwQQ+!+^VYIOluZH$t1c
> zv_+1>8TU`%w`h)@(r1P{H9j*3UWMt`>Zdm|)2X6NuZf^zo6HT^33OFGVROm+xg7+E
> z2zCH7=Ckn~Ont*evC3fii~+a6oEiR<l_BI{pd23a10>+%(6C)pm4FP?+`FUA;S7-i
> zZiP37&v5?)j3D5qZIGrDZbozFhC&IUcK{`{6hRaR2~lxZu$nN#0oxUZOqYc9coc$<
> z2KQ}~V9L13bfQTjNxH+a=XSz}5QH%U!bRJcg|uau^NDTIClmU4(b%zrS(#uaHZd4S
> zA<hRhFKkyPg=*Y~fH at 9<*r<R|I+IcCK}Lk)k$uC&VuJh4jL(-<LUUP>?;>M15qfhp
> zpJ4h2hDQQK6c%?JenYWka(7~rYQah3NFCRWG5^ejW^v at WJVdEuX~-#F=ImrCm>H<X
> zSeQRHB8q?}H*(7PBncJ+H?+|o7=VKBs?ya|jtd0_{kHyI))Y_&aH;j*sV$`iMR|bF
> z#WK8C^^5*c2i&2_J^cgp+aUKoz;9}=B`mM}eyJ&g0rWLcyO$kRX_Z0kH9NzW+ItJ<
> zNZRY`&`~jp5m)aEo(Y<&AYGeKx{B7kz!~{m?Wp>PGPG5_N3d<gmEls>bhSC`$QoP@
> zwaflMK_Oa1*z5PyURy(6N>}OWRpgCxpuEEe7+lDmj$kMcQ4LK<+3a6_(A2jVI$ZQS
> zEkzp;%(;Rh%I6&=W{3(lJF?m>Lrd<;Zxun<M<7jXOJsb1p@=DDDfdK<H+8k&BNH|I
> zJzc}cGHR^p3+ngkKq-T)sRJ at jTk9hSvMy4P{R9Y-_LLZijKAz7+(pmBfs#qoQsfT8
> z4M+~0AdGZB`T6 at sO_E+DF$J%JaANc7?Df(VmC4n)iDQqK=R$kVXhy1Gbg?;AK>sc-
> znV2j=$o;f-?gW_7k&EfZZ5MMLMR+k`mhCchen;uaT?LA at aPHNJx+oCaOE`;){1gtx
> zCR$Qyjk^6|PahF16jQ2^pbDztZ5V?qOK<@gzHi(?V1Hq2F)@Tk1NGkumi)g!E+olQ
> zAH~$?k!4|%S?DG}5UibVMu<bjJzcbdt5vw`t4H!(p9%)=u+JG|1jB+V3YRI2MpYr@
> zm1}Io3atqlt5LlF4C;s{ygL)6&=pl3>o^u6pBa-!h-p{#dD2=D`ZKpDVt4F>i4hv5
> zUFlvb2=(?HwA5%ESrCsi=nXraQ>j4498s@`mq)_5LZU*DUla3!kd{afJ4EB=N32Id
> zgv6fZ(Rb9T at 3FZ52(x2p6&hjR5mFZ{59ut9n at VYjmZ9hucN22u7Q2TyF<6G}L=@`S
> z(;_ at Y%n%o;(PE^E0JE(U;l;}BN~vJ?Qcg<j0Y3LjMTI^Pg`^_(>4_x{d@`RfZS|u|
> zvg805N>oyQS26fywKLQdTrNDs(!OP}`z_lKV#B3vLST`QDNJV7GnlfF9!tNZZYaYq
> zOCG_Kh({JliIG)@a}SYy9!>Eyg}upv8*FuGpb2;5W5RmIvC5^4|7&#I8e<hQUaD*Q
> zj7}}UBR^Iv4qY;OvTY@*fBlslnAa6`xmqx-G_KEfKGt_~n`-fyZ^;kK at s3f)VH{_5
> zjx*L#)4MBW8BCFRU0!e{|0G*!MM#$CnmWy8w~5bk;Qx)A(r9SY>5-7}G{H at A{1fn*
> z3aQcQoLS1ryA&jpwQ0Tz+@`|=mgUrh1*UI%SmTx;P1;ow0zG`Yyeu8XAJrE;K~WEk
> z&-S_CdZNr_wJ4z&rSLzrHl+`=!*Y!2;~oD)^;**ye{3OJY2}-W_M8<>gijJOE4jM1
> z$ieccWx4by`O-WR`aY3h3o$SNDAv#Fry}NGt;l?uRJ`2<#r@)&$ONlI)pPEZGC!@<
> z(On9|oHcU;u at +z+o@GM|qjBjdD1z<`uKpEVLw9AOm{sV<Ofdipg%tB+s4D{<@SSMb
> z(UJO69t`3j#!ZPQ8d*da3S-RDPp7eT4-17K*119O5Kf5Pxe=PPr;SZXDVVLZM{J8#
> zE5IWwV3y;nCps>2#fxUP0bjhs;&Ru at A+FkhGBvTmLcJA{e%c<FbC0=bb=G&cI&$dT
> zu-&-Dh|<f<C`^xbn0Rw>DTDtp0~7eA_Z?ai?+lal`XxPOrz}O|N|3IuQS at RA@E<U4
> zKEcj?99H7qz%Ok#%7FhI=1#*IKJD&4?7(ZtUc_RZbD{=<X)0+VLolN&7xG&io26tu
> z?J(N9R|UxT^1LR6)pv!{!V1jVs<ezl(k&ed7OaDe$5+k(f~U{I$akrhK1e_M)^jt`
> zk-z`%8lM>X`?-01>-V3d<oBP0`oZzx at k_e@KRP(v-2eX*1MB%oda*A_?-XsI_Iqa-
> z4U#Tvbh<rDBV_wNS$(L6(=U=lWs09<KMF{N?fYi)0k&U3g~Rq6NirQ~xF?wTbEsGu
> z6Z(s at Z>s4JPEheBiSD6e0uUc5S+!c*zE5O%pr^<3#Ka3a$AwBj{KUQ|NDAcj$_y&z
> zm(QzADovb#N`>8utGTniT6JoNd+->h_S09geEw^S-0CXT`Q({-eDn9;Mq at Sq4{&*t
> j|Np>vkp#d`AIO`r8Jn>go3R<2@!jK3Lgh0d04M+e6E#B`
>
> literal 0
> HcmV?d00001

It is hard to review the actual fs_test command, because it is submitted in the form
of a tarball. 

There are basically 4 types of test definitions supported by Fuego:
1) wrapper of an existing test - this is used when there is an existing test, and we are just
integrating it into the Fuego system.  This includes either a tarball or a git repository reference.
This is the form you have submitted this test in.  This is also the form used when compilation is
needed.
2) all-in-one test - this consists of a test that includes all functionality in the fuego_test.sh
It is used for original tests which either only run on the host (for example, only test the
build of an item), or have just a few commands to run on target (e.g. using 'dd' as a benchmarking
tool).
3) original, small tests: this consists of a fuego_test.sh and just a few source files or scripts,
and is not a pre-existing test.  Functional.libxml is a good example of this.  It has no compiled
elements, and the test materials are copied directly from the $TEST_HOME directory.
4) batch files: these are tests which reference a list of other tests.

I'll save my review of fs_test for a future e-mail.  But if this is an original test that needs an online
home we may want to consider 2 options:
a) put the source into the directory  test/Fucntional.check_mount, and figure out a mechanism to integrate this
into Fuego differently (maybe by creating a new phase  'test_unpack', to handle local sources)
b) create a github repository for fs_test, and reference that from spec.json or fuego_test.sh

In either case, it is very helpful to have the source in plain text (rather than compressed tarball) in
order to review it.

>
> diff --git a/tests/Functional.check_mounts/fuego_test.sh b/tests/Functional.check_mounts/fuego_test.sh
> new file mode 100755
> index 0000000..7e93f6f
> --- /dev/null
> +++ b/tests/Functional.check_mounts/fuego_test.sh
> @@ -0,0 +1,19 @@
> +tarball=fs-test-1.1.tgz
> +
> +function test_build {
> +    make
> +}
> +
> +function test_deploy {
> +    put fs_test $BOARD_TESTDIR/fuego.$TESTDIR/
> +    put /fuego-rw/boards/rpi3/expected_mounts.txt $BOARD_TESTDIR/fuego.$TESTDIR/
This should be parameterized, and you should check to make sure the file exists:
EXPECTED_FILE=/fuego-rw/boards/$NODE_NAME/expected_mounts.txt
if [ -f "${EXPECTED_FILE}" ] ; then
   put "${EXPECTED_FILE}"
else
   abort_job "Please create file ${EXPECTED_FILE} with the list of expected mounts"
fi
> +}
> + 
> +function test_run {
> +    report "cd $BOARD_TESTDIR/fuego.$TESTDIR; \
> +        ./fs_test"
> +}
> +
> +function test_processing {
> +    log_compare "$TESTDIR" "0" "FAIL" "n"
> +}
> diff --git a/tests/Functional.check_mounts/spec.json b/tests/Functional.check_mounts/spec.json
> new file mode 100644
> index 0000000..65eab4d
> --- /dev/null
> +++ b/tests/Functional.check_mounts/spec.json
> @@ -0,0 +1,7 @@
> +{
> +    "testName": "Functional.check_mounts",
> +    "specs": {
> +        "default": {}
> +    }

There is no spec for creating a baseline of mounts.  It would be nice if there were a 
spec called 'save_baseline' which created the 'expected_mounts.txt' file using the mounts
that are currently on the system (and place it into /fuego-rw/boards/$NODE_NAME/)

There would be two potential usages of this:
1) when there was a pre-existing list of expected mounts
These could be submitted ahead of time for well-known boards (like raspberry pi or beaglebone).
For a new user with a matching board, just put the expected_mounts file in /fuego-rw/boards/<board>
and run the test as is.

2) when there was no pre-existing list of expected mounts
 - A user would verify that the mounts were correct (functional)
 - A user would run 'ftc run-test -b <board> -t Functional.check_mounts -s save_baseline
in order to save the current mounts to /fuego-rw/boards/<board>
 - From then on, as part of the CI loop the user would run the test as usual to  verify
that there has been no change to the expected mounts.

> +}
> +
> diff --git a/tests/Functional.check_mounts/test.yaml b/tests/Functional.check_mounts/test.yaml
> new file mode 100644
> index 0000000..57942b5
> --- /dev/null
> +++ b/tests/Functional.check_mounts/test.yaml
> @@ -0,0 +1,15 @@
> +fuego_package_version: 1
> +name: Functional.check_mounts
> +description: |
> +    Sample test that runs fs_test. This test
> +    comes with specs to check the expected mounts with system's mount.
This description could be improved.

I would reword to:
Run fs_test, using an 'expected_mounts.txt' file, in order to verify
that the mouned filesystems on the target board are as expected.

> +license: MIT
> +author: Kumar Thangavel <thangavel.k at hcl.com>
> +maintainer: Kumar Thangavel <thangavel.k at hcl.com>
> +version: 1.1
> +fuego_release: 1
> +type: Functional
> +tags: ['example']
> +data_files:
> + - fs-test-1.1.tgz
> + - spec.json
> -- 
> 2.17.1

Thanks,
 -- Tim



More information about the Fuego mailing list