Skip to content

Conversation

@ddeidda
Copy link
Contributor

@ddeidda ddeidda commented Sep 18, 2020

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@codecov
Copy link

codecov bot commented Sep 18, 2020

Codecov Report

Merging #2058 into develop will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #2058   +/-   ##
========================================
  Coverage    59.91%   59.91%           
========================================
  Files           39       39           
  Lines         5620     5620           
========================================
  Hits          3367     3367           
  Misses        2253     2253           

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 4075a6c...ec0eca2. Read the comment docs.

from assertpy import assert_that


@pytest.mark.dimensions("eu-west-1", "c4.xlarge", "alinux2", "slurm")
Copy link
Contributor

Choose a reason for hiding this comment

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

let's use c5.xlarge only. I think we use c4 only in cn regions.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think eu-west-1 regions is heavily used already. Shall we pick a region with fewer tests?

also we could select centos7 with one of the schedulers.

compute_instance_type = {{ instance }}
initial_queue_size = 1
max_queue_size = 1
maintain_initial_size = true
Copy link
Contributor

Choose a reason for hiding this comment

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

let's set this to false so that in case of slurm we have the bootstrap logic submit a job to the scheduler (this happens only when initial_size > min_size ==> maintain_initial_size = false)


@pytest.mark.dimensions("eu-west-1", "c4.xlarge", "alinux2", "slurm")
@pytest.mark.dimensions("eu-west-2", "c5.xlarge", "alinux2", "sge")
@pytest.mark.usefixtures("scheduler, os, region, test_datadir")
Copy link
Contributor

Choose a reason for hiding this comment

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

why test_datadir in the usefixtures? can't it be removed?

you also need to add instance to the fixtures.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Test_datadir is needed from pcluster_config_reader, isn't it?

Copy link
Contributor

@demartinofra demartinofra Sep 18, 2020

Choose a reason for hiding this comment

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

yes but pcluster_config_reader will get the fixture regardless of it

@ddeidda ddeidda force-pushed the wip/pvt_nw_test branch 6 times, most recently from 990c603 to 7c692ef Compare September 18, 2020 16:00
Signed-off-by: ddeidda <ddeidda@amazon.com>
@ddeidda ddeidda merged commit 1986149 into aws:develop Sep 18, 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.

2 participants