Add buffer profile test (initial config test) #296
Conversation
|
@yxieca , please review. |
|
One of three test cases currently fails due to recent change in minigraph parser (missing information about neighbor type). |
andriymoroz-mlnx
force-pushed
the
buffers_test_init
branch
from
6919771 to
f339ef8
Compare
There was a problem hiding this comment.
-
The comment on minigraph fileglob is not blocking, Please let me know if you can finish the change with this review?
-
This test has a targeted devices in mind, can you make it more generic? For the reasons of:
2.1. if there is new mellanox device not starting with msn27xx, can you include these devices in tests with minimum effort?
2.2 if there is new other vendor's device and they provide the j2 template, can they use the test infrastructure with minimum effort?
It is ok for comment 2 to come later as an improvement change.
|
|
||
| - set_fact: | ||
| port_name="Ethernet64" | ||
| minigr="[ 'minigraph0.xml', 'minigraph1.xml', 'minigraph2.xml']" |
There was a problem hiding this comment.
Can these 3 files be picked up by fileglob or similar mechanism, instead of explicitly setting them?
There was a problem hiding this comment.
2.1 the test can run on any device but will test msn27xx platform template only. Currently this is the only platform which uses generated buffers config and not static.
2.2 yes, with some updates it can be used for the other platforms
about fileglob: maybe, but in this test the order is important
There was a problem hiding this comment.
Andriy,
The ordering part is quite tricky. I am not confident that another developer changing it will understand the constraints clearly.
Do you think you can change the minigr list to a dictionary, you can explicitly name the comparison target in the dictionary?
Cheers,
Ying
|
Test needs to be updated after sonic-net/sonic-buildimage#1101 is merged |
Signed-off-by: Andriy Moroz <[email protected]>
andriymoroz-mlnx
force-pushed
the
buffers_test_init
branch
from
f339ef8 to
8759898
Compare
|
@yxieca , can you review it again? It is updated. |
|
|
||
| - block: | ||
| - name: Copy test files to DUT | ||
| copy: src={{ item }} dest=/tmp/ |
There was a problem hiding this comment.
why not copy the directory? directory_mode=yes?
| - "{{ minigr }}" | ||
|
|
||
| - name: Compare generated buffers configuration with the expected | ||
| shell: diff /tmp/{{ (item | basename | splitext)[0]}}.json /tmp/{{ (item | basename).split('_')[0] }}.json |
There was a problem hiding this comment.
Do you think you can merge this step with the previous step,execute generation and comparison in the same shell command to reduce number of ssh sessions needed for the test? If you do so, you can always generate target file with a same name and compare it to the right template.
Regards,
Ying
|
This test is no longer relevant as of changes done while this PR was waiting for merge. |
No description provided.