Skip to content

Conversation

maciaszczykm
Copy link
Member

Fixes #503 issue.


This change is Review on Reviewable

@codecov-io
Copy link

Current coverage is 85.99%

Merging #513 into master will not affect coverage as of 0947856

@@            master    #513   diff @@
======================================
  Files           95      95       
  Stmts          814     814       
  Branches         0       0       
  Methods          0       0       
======================================
  Hit            700     700       
  Partial          0       0       
  Missed         114     114       

Review entire Coverage Diff as of 0947856

Powered by Codecov. Updated on successful CI builds.

@maciaszczykm
Copy link
Member Author

@cheld Could you take a look?

| Concurrency | delete replication controller | edit pod count | | | | | | | |
| Factor | 1 | 2 | 3 | 4 | 5 | 6 | 7 | 8 | Comment |
|-------------------------------|-----------------|---------------------|--------------|-------------|------|-----------------|---|---|---------|
| Screen size | normal | mobile | narrow | middle size | wide | active resizing | | | |
Copy link
Contributor

Choose a reason for hiding this comment

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

For RC list page, the layouting is a very important feature. That's why we should test screen sizes in very detail. I don't see it for logs page. I would leave it to the general factor 'screen size'

Copy link
Member Author

Choose a reason for hiding this comment

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

I can move it to general factor, but I thought, that general factors should be the same for all sheets. Regarding screen size factor I think it's important due to toolbar and it's behaviour on smaller screens. I agree, that there are many cases for this factor, but this test should be quick anyways.

@cheld
Copy link
Contributor

cheld commented Mar 16, 2016

Think about the following factors:

  • log size small, large (1 GB). I am not sure what happens. Does the browser blow up? Does the API server cut somehow?
  • max. name length for pod and container (we had bugs with layout!)
  • concurrency. e.g. delete the pod that is currently displayed (because of hpa scale-down)
  • log lines, e.g. very long lines

That's all I could think off. Please investigate and add more factors.

@maciaszczykm
Copy link
Member Author

Think about the following factors:

  • log size small, large (1 GB). I am not sure what happens. Does the browser blow up? Does the API server cut somehow?
  • max. name length for pod and container (we had bugs with layout!)
  • concurrency. e.g. delete the pod that is currently displayed (because of hpa scale-down)
  • log lines, e.g. very long lines

That's all I could think off. Please investigate and add more factors.

I've added logs size, pod and container name factors. Concurrency won't have any impact, because we do not refresh view asynchronously. Regarding log lines length, we do not have any impact on it, and I think when we will be testing logs with large size (1GB), then we'll see all kinds of logs line length.

PTAL

@cheld
Copy link
Contributor

cheld commented Mar 16, 2016

About concurrency:

Steps:

  1. Open Logs => second browser tab opens
  2. Switch browser tab
  3. delete pod
  4. switch to 2nd tab that shows the log
  5. press refresh

=>

Internal Server Error (500)

pods "frontend-iei5o" not found

Do we consider this as a bug? Maybe ok for the moment. I am ok with skipping concurrency.

@cheld
Copy link
Contributor

cheld commented Mar 16, 2016

LGTM, I merge as I do not expect further comments.

cheld added a commit that referenced this pull request Mar 16, 2016
@cheld cheld merged commit 5128fbb into kubernetes:master Mar 16, 2016
@maciaszczykm maciaszczykm deleted the logs-fbt branch March 16, 2016 13:52
anvithks added a commit to anvithks/k8s-dashboard that referenced this pull request Sep 27, 2021
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