New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Wip tests #1259
Wip tests #1259
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making these tests org aware and improving tests in general, specially with authz it should work now.
Changes look fine, there is commented code in one of the test, please remove that.
} | ||
} | ||
|
||
// private static class DummyOrganisationAutorizationStrategy extends AuthorizationStrategy { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove the commented code
@@ -599,7 +601,7 @@ protected User login(String userId, String fullName, String email) throws IOExce | |||
|
|||
UserDetails d = Jenkins.getInstance().getSecurityRealm().loadUserByUsername(bob.getId()); | |||
|
|||
SecurityContextHolder.getContext().setAuthentication(new PrincipalAcegiUserToken(bob.getId(),bob.getId(),bob.getId(), d.getAuthorities(), bob.getId())); | |||
SecurityContextHolder.getContext().setAuthentication(new PrincipalAcegiUserToken(bob.getId(),bob.getId(),bob.getId(), d.getAuthorities(), new PrincipalSid(bob.getId()))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, so this was it. Thanks:)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope - just pushing what I have been trying to get this to work....
@@ -31,7 +31,7 @@ | |||
<jacoco.line.coverage>0.70</jacoco.line.coverage> | |||
<jacoco.missedclass.coverage>0.00</jacoco.missedclass.coverage> | |||
<hpi.dependencyResolution>runtime</hpi.dependencyResolution> | |||
<jenkins-test-harness.version>2.15</jenkins-test-harness.version> | |||
<jenkins-test-harness.version>2.23</jenkins-test-harness.version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We had trouble upgrading to latest test harness. Lets see if CI is happy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2.16 is needed for MockAuthorizationStrategy
@vivek often times James has noted that unit tests fail (I have seen it in the past, mostly it is a 403 error returned instead of a 200 in a unit test) - and a few others. Subsequent runs pass. Is this possibly related? A lot of these tests are failing due to 403 @vivek - which is what I see from time to time (CredentialsApiTests also fail) - is there some loading/race problem? |
the wiremock setup is completely broken. If you look at the test output The Github Folder fails
Fixing this issue (you are using 1.34of commons io rather than the 2.something that is required causes loads of the tests to fail as wiremock no longer sees the expected calls . |
public void canCreateWhenHavePermissionsOnDefaultOrg() throws Exception { | ||
MockAuthorizationStrategy authz = new MockAuthorizationStrategy(); | ||
authz.grant(Jenkins.ADMINISTER).everywhere().to(user.getId()); | ||
j.jenkins.setAuthorizationStrategy(authz); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the jwt token becomes invalid.
a call to jwtToken = getJwtToken(j.jenkins, "vivek", "vivek");
seems to fix the issue.
`
I'm not expecting a review of this - its just so I can trigger builds on the CI. |
@@ -54,12 +56,18 @@ | |||
public abstract class PipelineBaseTest{ | |||
private static final Logger LOGGER = LoggerFactory.getLogger(PipelineBaseTest.class); | |||
|
|||
public PipelineBaseTest() { | |||
@BeforeClass | |||
public static void enableJWT() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@michaelneale FYI this could have been the cause of some of the 403 vs 200 failures (although I have not verified this at all)
Improve the test coverage and demonstrate the issue with MBP and custom organizations. shouldFailOnValidation4 is the test that fails in this case.
Hmm, don't see such error in blueocean build log or when I build it locally. I wonder this is due to windows env? I am afraid if we are fixing issues that is specific to windows env and might make things worse otherwise. |
it is not in the build log - it is in the unit test output. This is not windows specific - you can see the cause of the issue with |
@jtnord Ah, right. its indexing where its failing. These tests do not check result of indexing that is why they went unnoticed. Thanks for the fix:) |
@vivek I have not fixed that - I did but then with the amount of wiremock failures I decided not to, and just add tests to cover the issues I am fixing :) |
@jtnord no problem. I will take care of it, I have opened https://issues.jenkins-ci.org/browse/JENKINS-45662. Thanks. |
all rebased into #1252 |
just a PR for @vivek