Change tests in ArrayList using Reflection API #52

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

Jump to bottom

Closed

ivanvirchenko wants to merge 1 commit into main from GP-49-RefactorArrayListTestUsingReflectionAPI

Closed

Change tests in ArrayList using Reflection API #52

ivanvirchenko wants to merge 1 commit into main from GP-49-RefactorArrayListTestUsingReflectionAPI

Conversation

ivanvirchenko

Copy link

@ivanvirchenko ivanvirchenko commented Jan 29, 2021

No description provided.


 Change tests in ArrayList using Reflection API

2414cab

tboychuk

tboychuk requested changes

Jan 29, 2021

View reviewed changes

Copy link

Contributor

@tboychuk tboychuk left a comment

Choose a reason for hiding this comment

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

@erneman1 overall looks good! 👍
Please address comments and don't forget to extract try-catch blocks from all tests.

2-0-data-structures-and-algorithms/src/test/java/com/bobocode/array_list/ArrayListTest.java

assertThat(arrayList.get(0)).isEqualTo(10);

assertThat(arrayList.get(1)).isEqualTo(15);

assertThat(arrayList.get(2)).isEqualTo(20);

Object[] elements = (Object[]) getObjectByName("elementData");

Copy link

Contributor

@tboychuk tboychuk Jan 29, 2021

Choose a reason for hiding this comment

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

@erneman1 it's not safe to get this object by name since people can use other names for this field. That's why you should find array type filed, as I did in PoC

2-0-data-structures-and-algorithms/src/test/java/com/bobocode/array_list/ArrayListTest.java

arrayList.add(15);

arrayList.add(20);

try {

Copy link

Contributor

@tboychuk tboychuk Jan 29, 2021

Choose a reason for hiding this comment

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

@erneman1 test should not contain these try-catch blocks. Please create a helper method setTestSize(size) and move this logic to that method. You can also mark that method with @SneakyThrows

2-0-data-structures-and-algorithms/src/test/java/com/bobocode/array_list/ArrayListTest.java

arrayList.add(10);

arrayList.add(15);

arrayList.add(20);

try {

Copy link

Contributor

@tboychuk tboychuk Jan 29, 2021

Choose a reason for hiding this comment

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

@erneman1 this logic looks like a good candidate for a test method:

 @SneakyThrows
 private void fillTestArray(Object... elements){
 getFieldByName("elementData").set(arrayList, elements);
 getFieldByName("size").set(arrayList, elements.length);
 }

2-0-data-structures-and-algorithms/src/test/java/com/bobocode/array_list/ArrayListTest.java

@@ -94,9 +109,16 @@ public void getLastOfEmptyList() {

public void listWithSpecificCapacity() {

Copy link

Contributor

@tboychuk tboychuk Jan 29, 2021

Choose a reason for hiding this comment

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

@erneman1 let's rename this method to createListWithSpecificArrayCapacity

2-0-data-structures-and-algorithms/src/test/java/com/bobocode/array_list/ArrayListTest.java

arrayList.add(10);

arrayList.add(15);

arrayList.add(20);

try {

Copy link

Contributor

@tboychuk tboychuk Jan 29, 2021

Choose a reason for hiding this comment

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

@erneman1 this logic is redundant, we just need to verify that array capacity equals the one we passed to the constructor. So please remove this logic as well as assertions, verify that the inner array length is correct.

2-0-data-structures-and-algorithms/src/test/java/com/bobocode/array_list/ArrayListTest.java

assertThat(elements[2]).isEqualTo(10);

assertThat(elements[3]).isEqualTo(78);

assertThat(size).isEqualTo(4);

}

@Test

@Order(22)

public void setElementByIndexEqualToSize() {

Copy link

Contributor

@tboychuk tboychuk Jan 29, 2021

Choose a reason for hiding this comment

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

@erneman1 let's rename this test method to setElementByIndexThrowsExceptionWhenIndexIsOutOfBound

2-0-data-structures-and-algorithms/src/test/java/com/bobocode/array_list/ArrayListTest.java

assertThat(elements[2]).isEqualTo(78);

assertThat(elements[1]).isEqualTo(69);

assertThat(size).isEqualTo(4);

assertThat(removedElement).isEqualTo(58);

}

@Test

@Order(25)

public void removeElementByIndexEqualToSize() {

Copy link

Contributor

@tboychuk tboychuk Jan 29, 2021

Choose a reason for hiding this comment

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

@erneman1 let's remant test method to removeElementByIndexThrowsExceptionWhenIndexEqualsSize

2-0-data-structures-and-algorithms/src/test/java/com/bobocode/array_list/ArrayListTest.java

int size = (int) getObjectByName("size");

assertThat(elements[3]).isEqualTo(78);

assertThat(size).isEqualTo(4);

assertThat(removedElement).isEqualTo(100);

assertThatExceptionOfType(IndexOutOfBoundsException.class)

Copy link

Contributor

@tboychuk tboychuk Jan 29, 2021

Choose a reason for hiding this comment

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

@erneman1 please remove this last assertsion

2-0-data-structures-and-algorithms/src/test/java/com/bobocode/array_list/ArrayListTest.java

@@ -287,7 +375,11 @@ public void removeLastElementByIndex() {

@Test

@Order(27)

public void removeElementByIndexOutOfBounds() {

Copy link

Contributor

@tboychuk tboychuk Jan 29, 2021

Choose a reason for hiding this comment

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

@erneman1 please rename to removeElementByIndexThrowsExceptionWhenIndexIsOutOfBounds

2-0-data-structures-and-algorithms/src/test/java/com/bobocode/array_list/ArrayListTest.java

arrayList.clear();

assertThatExceptionOfType(IndexOutOfBoundsException.class)

.isThrownBy(() -> arrayList.get(0));

}

private Object getObjectByName(String name) {

Copy link

Contributor

@tboychuk tboychuk Jan 29, 2021

Choose a reason for hiding this comment

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

@erneman1 please remove all unnecessary blank lines

shryhus

shryhus requested changes

Jan 29, 2021

View reviewed changes

Copy link

Contributor

@shryhus shryhus left a comment

Choose a reason for hiding this comment

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

With a new testing logic we can change our test ordering to a logical and natural sequence of the exercise performing. For instance performing all the "add" test firstly.

@tboychuk tboychuk requested a review from v-ghost03

January 29, 2021 15:32

@ivanvirchenko ivanvirchenko closed this

Jan 29, 2021

@ivanvirchenko ivanvirchenko deleted the GP-49-RefactorArrayListTestUsingReflectionAPI branch

January 29, 2021 18:13

Labels

None yet

3 participants

@ivanvirchenko @tboychuk @shryhus

Uh oh!

Change tests in ArrayList using Reflection API #52

Change tests in ArrayList using Reflection API #52

Uh oh!

Conversation

@ivanvirchenko ivanvirchenko commented Jan 29, 2021

Uh oh!

@tboychuk tboychuk left a comment

Choose a reason for hiding this comment

Uh oh!

@tboychuk tboychuk Jan 29, 2021

Choose a reason for hiding this comment

Uh oh!

@tboychuk tboychuk Jan 29, 2021

Choose a reason for hiding this comment

Uh oh!

@tboychuk tboychuk Jan 29, 2021

Choose a reason for hiding this comment

Uh oh!

@tboychuk tboychuk Jan 29, 2021

Choose a reason for hiding this comment

Uh oh!

@tboychuk tboychuk Jan 29, 2021

Choose a reason for hiding this comment

Uh oh!

@tboychuk tboychuk Jan 29, 2021

Choose a reason for hiding this comment

Uh oh!

@tboychuk tboychuk Jan 29, 2021

Choose a reason for hiding this comment

Uh oh!

@tboychuk tboychuk Jan 29, 2021

Choose a reason for hiding this comment

Uh oh!

@tboychuk tboychuk Jan 29, 2021

Choose a reason for hiding this comment

Uh oh!

@tboychuk tboychuk Jan 29, 2021

Choose a reason for hiding this comment

Uh oh!

@shryhus shryhus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!