feat:Add configuration management to dashboard and test infrastructure prototype#25
Conversation
bassosimone
changed the title
bassosimone
left a comment
bassosimone
left a comment
There was a problem hiding this comment.
I did a lightweight review and I like what I see in terms of code organization and structure. I like that you added tests, especially for conversion functions.
I am going to merge this and deploy, so it's easier for us to test what is it doing.
Thanks!
|
|
||
| def test_identify_download(self): | ||
| """Test download requirement identification.""" | ||
| assert identify_requirement_type("download_speed") == RequirementType.DOWNLOAD |
There was a problem hiding this comment.
I like that you introduced a RequirementType. Neat!
| "use cases": { | ||
| "video_streaming": { | ||
| "w": 1.0, | ||
| "network requirements": { |
There was a problem hiding this comment.
This is not a comment about the current pull request, rather a reminder for me for later. I think we should try to avoid spaces in JSON documents, because it's more idiomatic to have underscores and there are benefits in terms of JavaScript DX as well. I'll most likely circle around to this next time I notice/remember: I already cleaned up part of the JSONs but missed this part.
| def test_negative_packet_loss_display(self): | ||
| """Test handling of negative values.""" | ||
| # This should work but produce negative percentage | ||
| result = convert_packet_loss_for_display(-0.1) | ||
| assert result == -10.0 | ||
|
|
||
| def test_large_packet_loss_values(self): | ||
| """Test handling of values > 100%.""" | ||
| result = convert_packet_loss_for_display(2.0) | ||
| assert result == 200.0 |
There was a problem hiding this comment.
This comment is not a blocker. I'd like to know the rationale though. My initial instinct here would have been to either normalize to the boundary (0-100%) or to throw (signal a bug). Can you elaborate on why you chose this strategy?
There was a problem hiding this comment.
This is a fair callout - typically I have a standard checklist I use for making unit tests so I was just going down the line but it makes more sense to bound it to 0-100, I will update on my next PR
| else: # Datasets | ||
| font_sizes.append(DATASET_FONT_SIZE) | ||
|
|
||
| fig = go.Figure( |
There was a problem hiding this comment.
"go figure" LOL this reads funny
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| main() |
There was a problem hiding this comment.
This comment is not a blocker. This file starts being quite large. It may make sense to split the functions it uses thematically (unless this creates too much indirection) in the interest of not creating too large context windows for AI. We should maybe consider this.
There was a problem hiding this comment.
Yes definitely, I'm already finding I am replicating a lot of code unnecessarily in the Map portion so I am reorganizing to shorten the file and abstract away some of the functions so it can be repurposed more easily.
2 participants
This update performs the following:
- Updated code management: Clearly delineates constants, utility functions, UI rendering functions etc. for cleaner code management
- Creates a dataclass for state management: Instead of repeatedly modifying session state, I used a dataclass to contain the various session state values
- Created test infrastructure: Unit tests added for the state management and general functionality of the prototype, followed pattern in /library.
- Enhanced manual entry function: Created a simple and advanced version where user can modify measurements attributed to each dataset(Cloudflare etc.). Previously manually entered values were being entered for all datasets. Now the user can select between attributing to all or to select datasets.
- Created modify configurations section: This section allows the user to define thresholds for each use case, define weights given to each network requirement, adjust use case weights and adjust dataset weights.
- Modified View calculation details section
Removed the Manual Entry portion and created a section that displays user entered configurations that are sent to the calculate functions.