Skip to content

Switch poe updates - #605

Merged
pisuke merged 4 commits into
faucetsdn:masterfrom
jhughesoti:switch-poe-updates
Sep 4, 2020
Merged

pisuke merged 4 commits into
faucetsdn:masterfrom
jhughesoti:switch-poe-updates

Conversation

@jhughesoti

Copy link
Copy Markdown
Collaborator

No description provided.

@noursaidi
noursaidi requested review from grafnu and pisuke August 28, 2020 08:28
@grafnu

grafnu commented Aug 28, 2020

Copy link
Copy Markdown
Collaborator

Can you please squash and force-push your commit history? Having so many commits in the PR makes it hard to review (because the web-pages lists them all out).

@jhughesoti

Copy link
Copy Markdown
Collaborator Author

What's the proper way to avoid this and fix the history currently? Those are all coming from just branching and updating from upstream as per previous discussions of the git work flow.

The history is less important in my opinion however than the file differences that can be reviewed, which is only 3 files and it's very minimal.

@grafnu

grafnu commented Aug 28, 2020 via email

Copy link
Copy Markdown
Collaborator

@jhughesoti

Copy link
Copy Markdown
Collaborator Author

Attempted with no effect to commit history.

@codecov

codecov Bot commented Aug 28, 2020 •

Copy link
Copy Markdown

Codecov Report

Merging #605 into master will decrease coverage by 0.59%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #605      +/-   ##
==========================================
- Coverage   80.96%   80.37%   -0.60%     
==========================================
  Files          22       22              
  Lines        3720     3720              
==========================================
- Hits         3012     2990      -22     
- Misses        708      730      +22     
Flag Coverage Δ
#aux 74.22% <ø> (-0.14%) ⬇️
#base 75.77% <ø> (-0.14%) ⬇️
#dhcp 72.31% <ø> (-0.06%) ⬇️
#many 72.52% <ø> (-0.49%) ⬇️
#topo 72.50% <ø> (-0.14%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
daq/gateway.py 90.57% <0.00%> (-3.67%) ⬇️
daq/runner.py 87.93% <0.00%> (-1.89%) ⬇️
daq/stream_monitor.py 90.00% <0.00%> (-0.84%) ⬇️
daq/topology.py 96.53% <0.00%> (-0.21%) ⬇️
daq/host.py 90.79% <0.00%> (-0.16%) ⬇️

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 b452810...0509e12. Read the comment docs.

@grafnu

grafnu commented Aug 28, 2020 via email

Copy link
Copy Markdown
Collaborator

@jhughesoti

Copy link
Copy Markdown
Collaborator Author
git merge master
Already up to date.

 git log -n 1
commit 78512bd5a32f9c4df9714ad258f48ee825e95fb0 (HEAD -> switch-poe-updates, origin/switch-poe-updates)
Merge: ddb0ad4 b4ad528
Author: jhughesbiot <jonathan.hughes@buildingsiot.com>
Date:   Fri Aug 28 08:57:30 2020 -0700

    Merge branch 'switch-poe-updates' of https://github.com/jhughesbiot/daq into switch-poe-updates

git reset master
Unstaged changes after reset:
M       docs/device_report.md
M       subset/switches/src/main/java/switchtest/SwitchTest.java
M       subset/switches/test_switch

git log -n 1
commit b196439b26892edfe85e21746c8f3b82a77125e4 (HEAD -> switch-poe-updates, origin/master, origin/HEAD, master)
Merge: 53acd58 861eccd
Author: jhughesbiot <jonathan.hughes@buildingsiot.com>
Date:   Fri Aug 28 08:54:01 2020 -0700

    Merge branch 'master' of https://github.com/jhughesbiot/daq

git add .
git log -n 1
commit b196439b26892edfe85e21746c8f3b82a77125e4 (HEAD -> switch-poe-updates, origin/master, origin/HEAD, master)
Merge: 53acd58 861eccd
Author: jhughesbiot <jonathan.hughes@buildingsiot.com>
Date:   Fri Aug 28 08:54:01 2020 -0700

    Merge branch 'master' of https://github.com/jhughesbiot/daq

git commit -m 'history cleanup attempt'
[switch-poe-updates 3b7f2d4] history cleanup attempt
 3 files changed, 1 insertion(+), 50 deletions(-)

git log -n 1
commit 3b7f2d47958cf820dffd7070aad996c73b18cd4c (HEAD -> switch-poe-updates)
Author: jhughesbiot <jonathan.hughes@buildingsiot.com>
Date:   Fri Aug 28 09:30:39 2020 -0700

    history cleanup attempt

git push
To https://github.com/jhughesbiot/daq
 ! [rejected]        switch-poe-updates -> switch-poe-updates (non-fast-forward)
error: failed to push some refs to 'https://github.com/jhughesbiot/daq'
hint: Updates were rejected because the tip of your current branch is behind
hint: its remote counterpart. Integrate the remote changes (e.g.
hint: 'git pull ...') before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.

git push --force
Username for 'https://github.com': jhughesbiot
Password for 'https://jhughesbiot@github.com':
Enumerating objects: 23, done.
Counting objects: 100% (23/23), done.
Delta compression using up to 8 threads
Compressing objects: 100% (9/9), done.
Writing objects: 100% (12/12), 876 bytes | 125.00 KiB/s, done.
Total 12 (delta 7), reused 0 (delta 0)
remote: Resolving deltas: 100% (7/7), completed with 7 local objects.
To https://github.com/jhughesbiot/daq
 + 78512bd...3b7f2d4 switch-poe-updates -> switch-poe-updates (forced update)

@grafnu

grafnu commented Aug 28, 2020 via email

Copy link
Copy Markdown
Collaborator

@jhughesoti

Copy link
Copy Markdown
Collaborator Author

It was already in sync but looks like resetting the master before merging was the key. Cleaned up now

@grafnu grafnu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working through the git issue. It's one of those things that just slowly builds up over time so good to be able to clear out the slate from time to time!

Comment thread docs/device_report.md Outdated
|skip|connection.port_speed|Other|Other|No local IP has been set, check system config|
|pass|manual.test.name|Security|Recommended|Manual test - for testing|
|skip|poe.negotiation|Other|Other|No local IP has been set, check system config|
|skip|poe.power|Other|Other|No local IP has been set, check system config|

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test name should be poe.switch.power

@noursaidi

Copy link
Copy Markdown
Collaborator

@jhughesbiot, I've reviewed and have the following comments:

  1. Test name should consistently be poe.switch.power
  2. The poe test result is not being copied into the test report - the cause of this is seems to be the module_manifest still having the older tests. Once removed it should work.
  3. The readme should be updated to remove reference to the older tests
  4. The results for all tests including the POE power test says 'see log above', but there isn't any additional information.

@jhughesoti

Copy link
Copy Markdown
Collaborator Author

@noursaidi
I have made the necessary adjustments to the files that should generate expected results. Due to the issue I reported a few days ago (#620) the switch module is in a state that can't be tested or verified at this point. Seems something in the usi module has been broken but I do not know what.

@jhughesoti

Copy link
Copy Markdown
Collaborator Author

@noursaidi @pisuke
Issue previously referenced has been resolved and merged. Looks like all tests are passing with the updates so unless there's something else we need to update, this one should be good to go.

@pisuke

pisuke commented Sep 4, 2020

Copy link
Copy Markdown
Collaborator

@jhughesbiot @spectre-storm @noursaidi @henry54809 @pbatta I've just checked the module with a range of configurations and devices and there are a few things to be fixed before the test code can be merged.

  1. This PoE device report correctly states that the device is powered, but it misses to indicate the power in the summary. @jhughesbiot can you please add the power in the summary?
  2. The same report shows that the output text in the switch section does not include any output. @jhughesbiot can you please add the full test output in the switch section?
  3. This non PoE device report correctly shows a skip because the device is configured as non-PoE, but the overall test summary is reported as a fail because the skip seems to be counted negatively. @henry54809 @pbatta I think that the logic in the creation of the table should omit the skips and only fail when there are true failures.
  4. @jhughesbiot there is a little typo poE that should become PoE. Can you please fix it?

After fixing 1, 2 and 4 we can merge the PR. Point 3 should be addressed in a separate PR.

@jhughesoti

Copy link
Copy Markdown
Collaborator Author

@pisuke
1 and 2
See discussion here #620 (comment) around why there are no logs in the switch test module. This was broken before any addition of the PoE updates and needs to be it's own separate PR to reintroduce this feature back to the report as this is a break when the usi was introduced, not PoE changes.

  1. updated

@pisuke

pisuke commented Sep 4, 2020

Copy link
Copy Markdown
Collaborator

Thanks @jhughesbiot, I'll get this merged then.
@henry54809 @spectre-storm issue #620 needs to be addressed as a priority, can you raise a bug for this please? I've moved the issue on the project kanban for our tracking on github. Thanks!

@pisuke
pisuke merged commit 9ce0f14 into faucetsdn:master Sep 4, 2020
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.

4 participants